[RFC PATCH 0/3] Remove mrlock

[RFC PATCH 0/3] Remove mrlock

[Nikolay Borisov]

This series removes mrlock_t and directly replaces i_lock and i_mmaplock with
rw_semaphore in xfs_inode. My end game with this is to eventually lift i_mmaplock
in VFS and use it from there. The necessity for the latter came up since BTRFS
is also about to get its own private version of i_mmaplock for the same use case.
This  will mean that all 3 major filesystems on linux (ext4/xfs/btrfs) wil share
the same lock. Christoph naturally suggested for the lock to be lifted to VFS.

Before proceeding with this work I'd like to get the opinion of XFS developers
whether doing that is acceptable for them. I've heard that Dave wants to eventually
convert the mmapsem to a range lock for XFS and implement a callback mechanism
for VFS to call into every filesystem...

I've only compile tested this and also the way the rwsem is checked for write
is admittedly a bit hackish but it can easily be changed to utilize lockdep.
I'm aware of https://lore.kernel.org/linux-xfs/20201102194135.174806-1-preichl@redhat.com/
but frankly that series went too far up to rev 10 which is a bit mind boggling...

Nikolay Borisov (3):
  xfs: Add is_rwsem_write_locked function
  xfs: Convert i_lock/i_mmaplock to  rw_semaphore
  xfs: Remove mrlock

 fs/xfs/mrlock.h          | 78 ----------------------------------------
 fs/xfs/xfs_inode.c       | 48 ++++++++++++++-----------
 fs/xfs/xfs_inode.h       |  6 ++--
 fs/xfs/xfs_linux.h       |  1 -
 fs/xfs/xfs_qm_syscalls.c |  2 +-
 fs/xfs/xfs_super.c       |  7 ++--
 6 files changed, 34 insertions(+), 108 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

--
2.25.1

[RFC PATCH 1/3] xfs: Add is_rwsem_write_locked function

[Nikolay Borisov]

This is going to be used to check if an rwsem is held for write.
Mimicking what current mrlock->mr_writer variable provides but using
the generic rwsem infrastructure. One might argue that redefining the
locked bit is a layering violation since RWSEM_WRITE_LOCKED is not
exposed, however I'd argue back that this is only used in a debugging
scenario and the physical layout of the rwsem is unlikely to change.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/xfs_inode.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index b7352bc4c815..499e63da935c 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -345,6 +345,13 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
+static bool is_rwsem_write_locked(struct rw_semaphore *rwsem)
+{
+	/* Copy of RWSEM_WRITE_LOCKED from rwsem.c */
+#define LOCK_BIT (1UL << 0)
+	return atomic_long_read(&rwsem->count) & LOCK_BIT;
+}
+
 int
 xfs_isilocked(
 	xfs_inode_t		*ip,
-- 
2.25.1

[RFC PATCH 2/3] xfs: Convert i_lock/i_mmaplock to rw_semaphore

[Nikolay Borisov]

Having the mrlock_t doesn't provide any benefit since rwsem is perfectly
equipped to answer the question whether a rwsem is held for write or
not. Exploit this fact to convert the i_lock/i_mmaplock to direct
semaphores, thus making mrlock unused. This removes a level of
indirection internal to xfs but also paves the way to lifting i_mmaplock
to VFS in the future.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/xfs_inode.c | 41 +++++++++++++++++++++--------------------
 fs/xfs/xfs_inode.h |  6 +++---
 fs/xfs/xfs_super.c |  7 ++-----
 3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 499e63da935c..dcf216eb7505 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -4,6 +4,7 @@
  * All Rights Reserved.
  */
 #include <linux/iversion.h>
+#include <linux/rwsem.h>
 
 #include "xfs.h"
 #include "xfs_fs.h"
@@ -191,14 +192,14 @@ 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);
 
@@ -359,14 +360,14 @@ xfs_isilocked(
 {
 	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);
+			return is_rwsem_write_locked(&ip->i_lock);
+		return rwsem_is_locked(&ip->i_lock);
 	}
 
 	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
 		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
-			return !!ip->i_mmaplock.mr_writer;
-		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
+			return is_rwsem_write_locked(&ip->i_mmaplock);
+		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 eca333f5f715..dd1d81afc4ce 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 */
 
 	/*
@@ -310,7 +310,7 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
  * 5		PARENT subclass (not nestable)
  * 6		RTBITMAP subclass (not nestable)
  * 7		RTSUM subclass (not nestable)
- * 
+ *
  */
 #define XFS_IOLOCK_SHIFT		16
 #define XFS_IOLOCK_MAX_SUBCLASS		3
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 813be879a5e5..7646001324c3 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -709,11 +709,8 @@ xfs_fs_inode_init_once(
 	/* xfs inode */
 	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.25.1

[RFC PATCH 3/3] xfs: Remove mrlock

[Nikolay Borisov]

It's no longer used so let's put the final nail in its coffin.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/mrlock.h          | 78 ----------------------------------------
 fs/xfs/xfs_linux.h       |  1 -
 fs/xfs/xfs_qm_syscalls.c |  2 +-
 3 files changed, 1 insertion(+), 80 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
deleted file mode 100644
index 79155eec341b..000000000000
--- a/fs/xfs/mrlock.h
+++ /dev/null
@@ -1,78 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2000-2006 Silicon Graphics, Inc.
- * All Rights Reserved.
- */
-#ifndef __XFS_SUPPORT_MRLOCK_H__
-#define __XFS_SUPPORT_MRLOCK_H__
-
-#include <linux/rwsem.h>
-
-typedef struct {
-	struct rw_semaphore	mr_lock;
-#if defined(DEBUG) || defined(XFS_WARN)
-	int			mr_writer;
-#endif
-} mrlock_t;
-
-#if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(mrp, name)	\
-	do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
-#else
-#define mrinit(mrp, name)	\
-	do { init_rwsem(&(mrp)->mr_lock); } while (0)
-#endif
-
-#define mrlock_init(mrp, t,n,s)	mrinit(mrp, n)
-#define mrfree(mrp)		do { } while (0)
-
-static inline void mraccess_nested(mrlock_t *mrp, int subclass)
-{
-	down_read_nested(&mrp->mr_lock, subclass);
-}
-
-static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
-{
-	down_write_nested(&mrp->mr_lock, subclass);
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-}
-
-static inline int mrtryaccess(mrlock_t *mrp)
-{
-	return down_read_trylock(&mrp->mr_lock);
-}
-
-static inline int mrtryupdate(mrlock_t *mrp)
-{
-	if (!down_write_trylock(&mrp->mr_lock))
-		return 0;
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-	return 1;
-}
-
-static inline void mrunlock_excl(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	up_write(&mrp->mr_lock);
-}
-
-static inline void mrunlock_shared(mrlock_t *mrp)
-{
-	up_read(&mrp->mr_lock);
-}
-
-static inline void mrdemote(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	downgrade_write(&mrp->mr_lock);
-}
-
-#endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 5b7a1e201559..fa57f2f060ca 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_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index ca1b57d291dc..1a582f21c619 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -206,7 +206,7 @@ xfs_qm_scall_quotaoff(
 	/*
 	 * Next we make the changes in the quota flag in the mount struct.
 	 * This isn't protected by a particular lock directly, because we
-	 * don't want to take a mrlock every time we depend on quotas being on.
+	 * don't want to take a rwsem every time we depend on quotas being on.
 	 */
 	mp->m_qflags &= ~flags;
 
-- 
2.25.1

[RFC PATCH 3/3] xfs: Remove mrlock

[Nikolay Borisov]

It's no longer used so let's put the final nail in its coffin.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/mrlock.h          | 78 ----------------------------------------
 fs/xfs/xfs_linux.h       |  1 -
 fs/xfs/xfs_qm_syscalls.c |  2 +-
 3 files changed, 1 insertion(+), 80 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
deleted file mode 100644
index 79155eec341b..000000000000
--- a/fs/xfs/mrlock.h
+++ /dev/null
@@ -1,78 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2000-2006 Silicon Graphics, Inc.
- * All Rights Reserved.
- */
-#ifndef __XFS_SUPPORT_MRLOCK_H__
-#define __XFS_SUPPORT_MRLOCK_H__
-
-#include <linux/rwsem.h>
-
-typedef struct {
-	struct rw_semaphore	mr_lock;
-#if defined(DEBUG) || defined(XFS_WARN)
-	int			mr_writer;
-#endif
-} mrlock_t;
-
-#if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(mrp, name)	\
-	do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
-#else
-#define mrinit(mrp, name)	\
-	do { init_rwsem(&(mrp)->mr_lock); } while (0)
-#endif
-
-#define mrlock_init(mrp, t,n,s)	mrinit(mrp, n)
-#define mrfree(mrp)		do { } while (0)
-
-static inline void mraccess_nested(mrlock_t *mrp, int subclass)
-{
-	down_read_nested(&mrp->mr_lock, subclass);
-}
-
-static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
-{
-	down_write_nested(&mrp->mr_lock, subclass);
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-}
-
-static inline int mrtryaccess(mrlock_t *mrp)
-{
-	return down_read_trylock(&mrp->mr_lock);
-}
-
-static inline int mrtryupdate(mrlock_t *mrp)
-{
-	if (!down_write_trylock(&mrp->mr_lock))
-		return 0;
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-	return 1;
-}
-
-static inline void mrunlock_excl(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	up_write(&mrp->mr_lock);
-}
-
-static inline void mrunlock_shared(mrlock_t *mrp)
-{
-	up_read(&mrp->mr_lock);
-}
-
-static inline void mrdemote(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	downgrade_write(&mrp->mr_lock);
-}
-
-#endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 5b7a1e201559..fa57f2f060ca 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_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index ca1b57d291dc..1a582f21c619 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -206,7 +206,7 @@ xfs_qm_scall_quotaoff(
 	/*
 	 * Next we make the changes in the quota flag in the mount struct.
 	 * This isn't protected by a particular lock directly, because we
-	 * don't want to take a mrlock every time we depend on quotas being on.
+	 * don't want to take a rwsem every time we depend on quotas being on.
 	 */
 	mp->m_qflags &= ~flags;
 
-- 
2.25.1

Re: [RFC PATCH 0/3] Remove mrlock

[Christoph Hellwig]

Pavel has looked into this before and got stuck on the allocator
workqueue offloads:

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

On Wed, Jan 13, 2021 at 01:17:03PM +0200, Nikolay Borisov wrote:
> This series removes mrlock_t and directly replaces i_lock and i_mmaplock with
> rw_semaphore in xfs_inode. My end game with this is to eventually lift i_mmaplock
> in VFS and use it from there. The necessity for the latter came up since BTRFS
> is also about to get its own private version of i_mmaplock for the same use case.
> This  will mean that all 3 major filesystems on linux (ext4/xfs/btrfs) wil share
> the same lock. Christoph naturally suggested for the lock to be lifted to VFS.
> 
> Before proceeding with this work I'd like to get the opinion of XFS developers
> whether doing that is acceptable for them. I've heard that Dave wants to eventually
> convert the mmapsem to a range lock for XFS and implement a callback mechanism
> for VFS to call into every filesystem...
> 
> I've only compile tested this and also the way the rwsem is checked for write
> is admittedly a bit hackish but it can easily be changed to utilize lockdep.
> I'm aware of https://lore.kernel.org/linux-xfs/20201102194135.174806-1-preichl@redhat.com/
> but frankly that series went too far up to rev 10 which is a bit mind boggling...
> 
> Nikolay Borisov (3):
>   xfs: Add is_rwsem_write_locked function
>   xfs: Convert i_lock/i_mmaplock to  rw_semaphore
>   xfs: Remove mrlock
> 
>  fs/xfs/mrlock.h          | 78 ----------------------------------------
>  fs/xfs/xfs_inode.c       | 48 ++++++++++++++-----------
>  fs/xfs/xfs_inode.h       |  6 ++--
>  fs/xfs/xfs_linux.h       |  1 -
>  fs/xfs/xfs_qm_syscalls.c |  2 +-
>  fs/xfs/xfs_super.c       |  7 ++--
>  6 files changed, 34 insertions(+), 108 deletions(-)
>  delete mode 100644 fs/xfs/mrlock.h
> 
> --
> 2.25.1
> 
---end quoted text---

Re: [RFC PATCH 0/3] Remove mrlock

[Nikolay Borisov]

On 13.01.21 г. 13:27 ч., Christoph Hellwig wrote:
> Pavel has looked into this before and got stuck on the allocator
> workqueue offloads:
> 
> [PATCH v13 0/4] xfs: Remove wrappers for some semaphores

I haven't looked into his series but I fail to see how lifting
rwsemaphore out of the nested structure can change the behavior ? It
just removes a level of indirection. My patches are semantically
identical to the original code.

<snip>

Re: [RFC PATCH 0/3] Remove mrlock

[Christoph Hellwig]

On Wed, Jan 13, 2021 at 01:41:09PM +0200, Nikolay Borisov wrote:
> 
> 
> On 13.01.21 ??. 13:27 ??., Christoph Hellwig wrote:
> > Pavel has looked into this before and got stuck on the allocator
> > workqueue offloads:
> > 
> > [PATCH v13 0/4] xfs: Remove wrappers for some semaphores
> 
> I haven't looked into his series but I fail to see how lifting
> rwsemaphore out of the nested structure can change the behavior ? It
> just removes a level of indirection. My patches are semantically
> identical to the original code.

mrlocks have the mr_writer field that annotate that the is a writer
locking the lock.  The XFS asserts use it to assert that the lock that
the current thread holds it for exclusive protection, which isn't
actually what the field says, and this breaks when XFS uses synchronous
execution of work_struct as basically an extension of the kernel stack.

Re: [RFC PATCH 0/3] Remove mrlock

[Nikolay Borisov]

On 13.01.21 г. 14:09 ч., Christoph Hellwig wrote:
> On Wed, Jan 13, 2021 at 01:41:09PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 13.01.21 ??. 13:27 ??., Christoph Hellwig wrote:
>>> Pavel has looked into this before and got stuck on the allocator
>>> workqueue offloads:
>>>
>>> [PATCH v13 0/4] xfs: Remove wrappers for some semaphores
>>
>> I haven't looked into his series but I fail to see how lifting
>> rwsemaphore out of the nested structure can change the behavior ? It
>> just removes a level of indirection. My patches are semantically
>> identical to the original code.
> 
> mrlocks have the mr_writer field that annotate that the is a writer
> locking the lock.  The XFS asserts use it to assert that the lock that
> the current thread holds it for exclusive protection, which isn't
> actually what the field says, and this breaks when XFS uses synchronous
> execution of work_struct as basically an extension of the kernel stack.

I'm still failing to see what's the problem of checking the last bit of
the rwsem ->count field. It is set when the sem is held for writing
(identical to what mr_write does). As I mention in the cover letter this
might be considered a bit hacky because it exposes an internal detail of
the rwsem i.e that the bit of interest is bit 0. But I believe the same
can be achieved using lockdep_is_held_type(xx, 0/1) and making XFS's
debug routines depend on lockdep being on.

> 

Re: [RFC PATCH 0/3] Remove mrlock

[Darrick J. Wong]

On Wed, Jan 13, 2021 at 02:17:29PM +0200, Nikolay Borisov wrote:
> 
> 
> On 13.01.21 г. 14:09 ч., Christoph Hellwig wrote:
> > On Wed, Jan 13, 2021 at 01:41:09PM +0200, Nikolay Borisov wrote:
> >>
> >>
> >> On 13.01.21 ??. 13:27 ??., Christoph Hellwig wrote:
> >>> Pavel has looked into this before and got stuck on the allocator
> >>> workqueue offloads:
> >>>
> >>> [PATCH v13 0/4] xfs: Remove wrappers for some semaphores
> >>
> >> I haven't looked into his series but I fail to see how lifting
> >> rwsemaphore out of the nested structure can change the behavior ? It
> >> just removes a level of indirection. My patches are semantically
> >> identical to the original code.
> > 
> > mrlocks have the mr_writer field that annotate that the is a writer
> > locking the lock.  The XFS asserts use it to assert that the lock that
> > the current thread holds it for exclusive protection, which isn't
> > actually what the field says, and this breaks when XFS uses synchronous
> > execution of work_struct as basically an extension of the kernel stack.
> 
> I'm still failing to see what's the problem of checking the last bit of
> the rwsem ->count field. It is set when the sem is held for writing
> (identical to what mr_write does). As I mention in the cover letter this
> might be considered a bit hacky because it exposes an internal detail of
> the rwsem i.e that the bit of interest is bit 0.

I don't want to tear into the implementation details of rwsems if I can
avoid it.  Just because we all have one big happy address space doesn't
mean shortcuts won't hose everyone.

> But I believe the same
> can be achieved using lockdep_is_held_type(xx, 0/1) and making XFS's
> debug routines depend on lockdep being on.

Pavel Reichl tried that two months ago in:
https://lore.kernel.org/linux-xfs/20201102194135.174806-2-preichl@redhat.com/

which resulted in fstests regressions:
https://lore.kernel.org/linux-xfs/20201104005345.GC7115@magnolia/

TLDR: the ILOCK semaphore is data-centric, but lockdep's debugging
chains are task-centric, which causes incorrect lock validation reports.

The solutions as I see them are: (a) figure out if we really still need
to defer bmbt splits to workers to avoid overflowing the kernel stack;
or (b) making it possible to transfer rwsem ownership to shut up
lockdep; or (c) fix the is_held predicate to ignore ownership.

--D

> 
> >