* [PATCH v2 0/5] Remove the XFS mrlock
@ 2023-10-07 20:35 Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write() Matthew Wilcox (Oracle)
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
XFS has an mrlock wrapper around the rwsem which adds only the
functionality of knowing whether the rwsem is currently held in read
or write mode. Both regular rwsems and rt-rwsems know this, they just
don't expose it as an API. By adding that, we can remove the XFS mrlock
as well as improving the debug assertions for the mmap_lock when lockdep
is disabled.
v2: Add rwsem_assert_held() and rwsem_assert_held_write() instead of
augmenting the existing rwsem_is_locked() with rwsem_is_write_locked().
There's also an __rwsem_assert_held() and __rwsem_assert_held_write()
for the benefit of XFS when it's in a context where lockdep doesn't
know what's going on. It's still an improvement, so I hope those who
are looking for perfection can accept a mere improvement.
We can do more to replace uses of rwsem_is_locked(), and I have a few of
those in my tree, but let's focus on these three use cases for now and
we can trickle in other improvements through other maintainers after 6.7.
I'm sympathetic to "this will warn twice and dump much the same
information if you have lockdep enabled". Perhaps somebody has a
suggestion for not doing that?
Matthew Wilcox (Oracle) (5):
locking: Add rwsem_assert_held() and rwsem_assert_held_write()
mm: Use rwsem assertion macros for mmap_lock
xfs: Replace xfs_isilocked with xfs_assert_locked
xfs: Remove mrlock wrapper
fs: Add inode_assert_locked() and inode_assert_locked_excl()
fs/attr.c | 2 +-
fs/namei.c | 6 +--
fs/xfs/libxfs/xfs_attr.c | 2 +-
fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
fs/xfs/libxfs/xfs_bmap.c | 19 ++++----
fs/xfs/libxfs/xfs_defer.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
fs/xfs/libxfs/xfs_trans_inode.c | 6 +--
fs/xfs/mrlock.h | 78 ------------------------------
fs/xfs/scrub/readdir.c | 4 +-
fs/xfs/xfs_attr_list.c | 2 +-
fs/xfs/xfs_bmap_util.c | 10 ++--
fs/xfs/xfs_dir2_readdir.c | 2 +-
fs/xfs/xfs_dquot.c | 4 +-
fs/xfs/xfs_file.c | 4 +-
fs/xfs/xfs_inode.c | 86 ++++++++++++---------------------
fs/xfs/xfs_inode.h | 4 +-
fs/xfs/xfs_inode_item.c | 4 +-
fs/xfs/xfs_iops.c | 7 ++-
fs/xfs/xfs_linux.h | 2 +-
fs/xfs/xfs_qm.c | 10 ++--
fs/xfs/xfs_reflink.c | 2 +-
fs/xfs/xfs_rtalloc.c | 4 +-
fs/xfs/xfs_super.c | 4 +-
fs/xfs/xfs_symlink.c | 2 +-
fs/xfs/xfs_trans_dquot.c | 2 +-
include/linux/fs.h | 10 ++++
include/linux/mmap_lock.h | 10 ++--
include/linux/rwbase_rt.h | 9 +++-
include/linux/rwsem.h | 42 ++++++++++++++--
31 files changed, 142 insertions(+), 203 deletions(-)
delete mode 100644 fs/xfs/mrlock.h
--
2.40.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write()
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
@ 2023-10-07 20:35 ` Matthew Wilcox (Oracle)
2023-10-08 21:54 ` Dave Chinner
2023-10-07 20:35 ` [PATCH v2 2/5] mm: Use rwsem assertion macros for mmap_lock Matthew Wilcox (Oracle)
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
Modelled after lockdep_assert_held() and lockdep_assert_held_write(),
but are always active, even when lockdep is disabled. Of course, they
don't test that _this_ thread is the owner, but it's sufficient to catch
many bugs and doesn't incur the same performance penalty as lockdep.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
include/linux/rwbase_rt.h | 9 +++++++--
include/linux/rwsem.h | 42 ++++++++++++++++++++++++++++++++++-----
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
index 1d264dd08625..a04acd85705b 100644
--- a/include/linux/rwbase_rt.h
+++ b/include/linux/rwbase_rt.h
@@ -26,12 +26,17 @@ struct rwbase_rt {
} while (0)
-static __always_inline bool rw_base_is_locked(struct rwbase_rt *rwb)
+static __always_inline bool rw_base_is_locked(const struct rwbase_rt *rwb)
{
return atomic_read(&rwb->readers) != READER_BIAS;
}
-static __always_inline bool rw_base_is_contended(struct rwbase_rt *rwb)
+static inline void rw_base_assert_held_write(const struct rwbase_rt *rwb)
+{
+ BUG_ON(atomic_read(&rwb->readers) != WRITER_BIAS);
+}
+
+static __always_inline bool rw_base_is_contended(const struct rwbase_rt *rwb)
{
return atomic_read(&rwb->readers) > 0;
}
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 1dd530ce8b45..048149f781b3 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -66,14 +66,24 @@ struct rw_semaphore {
#endif
};
-/* In all implementations count != 0 means locked */
+#define RWSEM_UNLOCKED_VALUE 0UL
+#define RWSEM_WRITER_LOCKED (1UL << 0)
+#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE)
+
static inline int rwsem_is_locked(struct rw_semaphore *sem)
{
- return atomic_long_read(&sem->count) != 0;
+ return atomic_long_read(&sem->count) != RWSEM_UNLOCKED_VALUE;
}
-#define RWSEM_UNLOCKED_VALUE 0L
-#define __RWSEM_COUNT_INIT(name) .count = ATOMIC_LONG_INIT(RWSEM_UNLOCKED_VALUE)
+static inline void __rwsem_assert_held(const struct rw_semaphore *sem)
+{
+ WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE);
+}
+
+static inline void __rwsem_assert_held_write(const struct rw_semaphore *sem)
+{
+ WARN_ON(!(atomic_long_read(&sem->count) & RWSEM_WRITER_LOCKED));
+}
/* Common initializer macros and functions */
@@ -152,11 +162,21 @@ do { \
__init_rwsem((sem), #sem, &__key); \
} while (0)
-static __always_inline int rwsem_is_locked(struct rw_semaphore *sem)
+static __always_inline int rwsem_is_locked(const struct rw_semaphore *sem)
{
return rw_base_is_locked(&sem->rwbase);
}
+static inline void __rwsem_assert_held(const struct rw_semaphore *sem)
+{
+ BUG_ON(!rwsem_is_locked(sem));
+}
+
+static inline void __rwsem_assert_held_write(const struct rw_semaphore *sem)
+{
+ rw_base_assert_held_write(sem);
+}
+
static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
{
return rw_base_is_contended(&sem->rwbase);
@@ -169,6 +189,18 @@ static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
* the RT specific variant.
*/
+static inline void rwsem_assert_held(const struct rw_semaphore *sem)
+{
+ lockdep_assert_held(sem);
+ __rwsem_assert_held(sem);
+}
+
+static inline void rwsem_assert_held_write(const struct rw_semaphore *sem)
+{
+ lockdep_assert_held_write(sem);
+ __rwsem_assert_held_write(sem);
+}
+
/*
* lock for reading
*/
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] mm: Use rwsem assertion macros for mmap_lock
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write() Matthew Wilcox (Oracle)
@ 2023-10-07 20:35 ` Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked Matthew Wilcox (Oracle)
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
This slightly strengthens our write assertion when lockdep is disabled.
It also downgrades us from BUG_ON to WARN_ON, but I think that's an
improvement. I don't think dumping the mm_struct was all that valuable;
the call chain is what's important.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
include/linux/mmap_lock.h | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 8d38dcb6d044..de9dc20b01ba 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -60,16 +60,14 @@ static inline void __mmap_lock_trace_released(struct mm_struct *mm, bool write)
#endif /* CONFIG_TRACING */
-static inline void mmap_assert_locked(struct mm_struct *mm)
+static inline void mmap_assert_locked(const struct mm_struct *mm)
{
- lockdep_assert_held(&mm->mmap_lock);
- VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+ rwsem_assert_held(&mm->mmap_lock);
}
-static inline void mmap_assert_write_locked(struct mm_struct *mm)
+static inline void mmap_assert_write_locked(const struct mm_struct *mm)
{
- lockdep_assert_held_write(&mm->mmap_lock);
- VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
+ rwsem_assert_held_write(&mm->mmap_lock);
}
#ifdef CONFIG_PER_VMA_LOCK
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write() Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 2/5] mm: Use rwsem assertion macros for mmap_lock Matthew Wilcox (Oracle)
@ 2023-10-07 20:35 ` Matthew Wilcox (Oracle)
2023-10-08 21:59 ` Dave Chinner
2023-10-09 18:27 ` Darrick J. Wong
2023-10-07 20:35 ` [PATCH v2 4/5] xfs: Remove mrlock wrapper Matthew Wilcox (Oracle)
` (2 subsequent siblings)
5 siblings, 2 replies; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
To use the new rwsem_assert_held()/rwsem_assert_held_write(), we can't
use the existing ASSERT macro.
xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL) checks both the
IOLOCK and the ILOCK are held for write. xfs_isilocked() only checked
that the ILOCK was held for write.
xfs_assert_ilocked() is always on, even if DEBUG or XFS_WARN aren't
defined. It's a cheap check, so I don't think it's worth defining
it away.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/xfs/libxfs/xfs_attr.c | 2 +-
fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
fs/xfs/libxfs/xfs_bmap.c | 19 +++++----
fs/xfs/libxfs/xfs_defer.c | 2 +-
fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
fs/xfs/libxfs/xfs_trans_inode.c | 6 +--
fs/xfs/scrub/readdir.c | 4 +-
fs/xfs/xfs_attr_list.c | 2 +-
fs/xfs/xfs_bmap_util.c | 10 ++---
fs/xfs/xfs_dir2_readdir.c | 2 +-
fs/xfs/xfs_dquot.c | 4 +-
fs/xfs/xfs_file.c | 4 +-
fs/xfs/xfs_inode.c | 72 +++++++++++----------------------
fs/xfs/xfs_inode.h | 2 +-
fs/xfs/xfs_inode_item.c | 4 +-
fs/xfs/xfs_iops.c | 3 +-
fs/xfs/xfs_qm.c | 10 ++---
fs/xfs/xfs_reflink.c | 2 +-
fs/xfs/xfs_rtalloc.c | 4 +-
fs/xfs/xfs_symlink.c | 2 +-
fs/xfs/xfs_trans_dquot.c | 2 +-
22 files changed, 66 insertions(+), 96 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e28d93d232de..ebf0722d8963 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -224,7 +224,7 @@ int
xfs_attr_get_ilocked(
struct xfs_da_args *args)
{
- ASSERT(xfs_isilocked(args->dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(args->dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
if (!xfs_inode_hasattr(args->dp))
return -ENOATTR;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index d440393b40eb..1c007ebf153a 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -545,7 +545,7 @@ xfs_attr_rmtval_stale(
struct xfs_buf *bp;
int error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (XFS_IS_CORRUPT(mp, map->br_startblock == DELAYSTARTBLOCK) ||
XFS_IS_CORRUPT(mp, map->br_startblock == HOLESTARTBLOCK))
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 30c931b38853..d70cf543a52d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1189,7 +1189,7 @@ xfs_iread_extents(
if (!xfs_need_iread_extents(ifp))
return 0;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ir.loaded = 0;
xfs_iext_first(ifp, &ir.icur);
@@ -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));
+ xfs_assert_ilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL);
if (WARN_ON_ONCE(!ifp))
return -EFSCORRUPTED;
@@ -4354,7 +4354,7 @@ xfs_bmapi_write(
ASSERT(tp != NULL);
ASSERT(len > 0);
ASSERT(ifp->if_format != XFS_DINODE_FMT_LOCAL);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(!(flags & XFS_BMAPI_REMAP));
/* zeroing is for currently only for data extents, not metadata */
@@ -4651,7 +4651,7 @@ xfs_bmapi_remap(
ifp = xfs_ifork_ptr(ip, whichfork);
ASSERT(len > 0);
ASSERT(len <= (xfs_filblks_t)XFS_MAX_BMBT_EXTLEN);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
XFS_BMAPI_NORMAP)));
ASSERT((flags & (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC)) !=
@@ -5305,7 +5305,7 @@ __xfs_bunmapi(
if (xfs_is_shutdown(mp))
return -EIO;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(len > 0);
ASSERT(nexts >= 0);
@@ -5648,8 +5648,7 @@ xfs_bmse_merge(
blockcount = left->br_blockcount + got->br_blockcount;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
ASSERT(xfs_bmse_can_merge(left, got, shift));
new = *left;
@@ -5777,7 +5776,7 @@ xfs_bmap_collapse_extents(
if (xfs_is_shutdown(mp))
return -EIO;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
error = xfs_iread_extents(tp, ip, whichfork);
if (error)
@@ -5850,7 +5849,7 @@ xfs_bmap_can_insert_extents(
int is_empty;
int error = 0;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
if (xfs_is_shutdown(ip->i_mount))
return -EIO;
@@ -5892,7 +5891,7 @@ xfs_bmap_insert_extents(
if (xfs_is_shutdown(mp))
return -EIO;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
error = xfs_iread_extents(tp, ip, whichfork);
if (error)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index bcfb6a4203cd..7927a721dc86 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -744,7 +744,7 @@ xfs_defer_ops_capture(
* transaction.
*/
for (i = 0; i < dfc->dfc_held.dr_inos; i++) {
- ASSERT(xfs_isilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL);
ihold(VFS_I(dfc->dfc_held.dr_ip[i]));
}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 5a2e7ddfa76d..14193e044f3d 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -563,7 +563,7 @@ xfs_iextents_copy(
struct xfs_bmbt_irec rec;
int64_t copied = 0;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
ASSERT(ifp->if_bytes > 0);
for_each_xfs_iext(ifp, &icur, &rec) {
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index fa180ab66b73..4146b8697c89 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -974,7 +974,7 @@ xfs_rtfree_extent(
mp = tp->t_mountp;
ASSERT(mp->m_rbmip->i_itemp != NULL);
- ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
error = xfs_rtcheck_alloc_range(mp, tp, bno, len);
if (error)
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 6b2296ff248a..05229fdef36b 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -31,7 +31,7 @@ xfs_trans_ijoin(
{
struct xfs_inode_log_item *iip;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (ip->i_itemp == NULL)
xfs_inode_item_init(ip, ip->i_mount);
iip = ip->i_itemp;
@@ -60,7 +60,7 @@ xfs_trans_ichgtime(
struct timespec64 tv;
ASSERT(tp);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
tv = current_time(inode);
@@ -90,7 +90,7 @@ xfs_trans_log_inode(
struct inode *inode = VFS_I(ip);
ASSERT(iip);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(!xfs_iflags_test(ip, XFS_ISTALE));
tp->t_flags |= XFS_TRANS_DIRTY;
diff --git a/fs/xfs/scrub/readdir.c b/fs/xfs/scrub/readdir.c
index e51c1544be63..0b200bab04c8 100644
--- a/fs/xfs/scrub/readdir.c
+++ b/fs/xfs/scrub/readdir.c
@@ -283,7 +283,7 @@ xchk_dir_walk(
return -EIO;
ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
- ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL)
return xchk_dir_walk_sf(sc, dp, dirent_fn, priv);
@@ -334,7 +334,7 @@ xchk_dir_lookup(
return -EIO;
ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
- ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) {
error = xfs_dir2_sf_lookup(&args);
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 99bbbe1a0e44..235dd125c92f 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -505,7 +505,7 @@ xfs_attr_list_ilocked(
{
struct xfs_inode *dp = context->dp;
- ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
/*
* Decide on what work routines to call based on the inode size.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fcefab687285..1bd430f69d44 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -655,8 +655,8 @@ xfs_can_free_eofblocks(
* Caller must either hold the exclusive io lock; or be inactivating
* the inode, which guarantees there are no other users of the inode.
*/
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL) ||
- (VFS_I(ip)->i_state & I_FREEING));
+ if (!(VFS_I(ip)->i_state & I_FREEING))
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
/* prealloc/delalloc exists only on regular files */
if (!S_ISREG(VFS_I(ip)->i_mode))
@@ -1112,8 +1112,7 @@ xfs_collapse_file_space(
xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
bool done = false;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
trace_xfs_collapse_file_space(ip);
@@ -1182,8 +1181,7 @@ xfs_insert_file_space(
xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
bool done = false;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
trace_xfs_insert_file_space(ip);
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 9f3ceb461515..95cd8b9cf3dc 100644
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -521,7 +521,7 @@ xfs_readdir(
return -EIO;
ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
- ASSERT(xfs_isilocked(dp, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
+ xfs_assert_ilocked(dp, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL);
XFS_STATS_INC(dp->i_mount, xs_dir_getdents);
args.dp = dp;
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index ac6ba646624d..4b2f1b82badc 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -949,7 +949,7 @@ xfs_qm_dqget_inode(
if (error)
return error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(xfs_inode_dquot(ip, type) == NULL);
id = xfs_qm_id_for_quotatype(ip, type);
@@ -1006,7 +1006,7 @@ xfs_qm_dqget_inode(
}
dqret:
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
trace_xfs_dqget_miss(dqp);
*O_dqpp = dqp;
return 0;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 203700278ddb..9ba2b89b3862 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -842,7 +842,7 @@ xfs_break_dax_layouts(
{
struct page *page;
- ASSERT(xfs_isilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL));
+ xfs_assert_ilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL);
page = dax_layout_busy_page(inode->i_mapping);
if (!page)
@@ -863,7 +863,7 @@ xfs_break_layouts(
bool retry;
int error;
- ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
+ xfs_assert_ilocked(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 4d55f58d99b7..812d6f255d84 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -333,52 +333,26 @@ xfs_ilock_demote(
trace_xfs_ilock_demote(ip, lock_flags, _RET_IP_);
}
-#if defined(DEBUG) || defined(XFS_WARN)
-static inline bool
-__xfs_rwsem_islocked(
- struct rw_semaphore *rwsem,
- bool shared)
-{
- if (!debug_locks)
- return rwsem_is_locked(rwsem);
-
- if (!shared)
- return lockdep_is_held_type(rwsem, 0);
-
- /*
- * We are checking that the lock is held at least in shared
- * mode but don't care that it might be held exclusively
- * (i.e. shared | excl). Hence we check if the lock is held
- * in any mode rather than an explicit shared mode.
- */
- return lockdep_is_held_type(rwsem, -1);
-}
-
-bool
-xfs_isilocked(
+void
+xfs_assert_ilocked(
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_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
- return __xfs_rwsem_islocked(&VFS_I(ip)->i_mapping->invalidate_lock,
- (lock_flags & XFS_MMAPLOCK_SHARED));
- }
-
- if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
- return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
- (lock_flags & XFS_IOLOCK_SHARED));
- }
-
- ASSERT(0);
- return false;
+ if (lock_flags & XFS_ILOCK_SHARED)
+ rwsem_assert_held(&ip->i_lock.mr_lock);
+ else if (lock_flags & XFS_ILOCK_EXCL)
+ BUG_ON(!ip->i_lock.mr_writer);
+
+ if (lock_flags & XFS_MMAPLOCK_SHARED)
+ rwsem_assert_held(&VFS_I(ip)->i_mapping->invalidate_lock);
+ else if (lock_flags & XFS_MMAPLOCK_EXCL)
+ rwsem_assert_held_write(&VFS_I(ip)->i_mapping->invalidate_lock);
+
+ if (lock_flags & XFS_IOLOCK_SHARED)
+ rwsem_assert_held(&VFS_I(ip)->i_rwsem);
+ else if (lock_flags & XFS_IOLOCK_EXCL)
+ rwsem_assert_held_write(&VFS_I(ip)->i_rwsem);
}
-#endif
/*
* xfs_lockdep_subclass_ok() is only used in an ASSERT, so is only called when
@@ -1335,9 +1309,9 @@ xfs_itruncate_extents_flags(
xfs_filblks_t unmap_len;
int error = 0;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
- ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
- xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
+ if (atomic_read(&VFS_I(ip)->i_count))
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
ASSERT(new_size <= XFS_ISIZE(ip));
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
ASSERT(ip->i_itemp != NULL);
@@ -1598,7 +1572,7 @@ xfs_inactive_ifree(
xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
error = xfs_ifree(tp, ip);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (error) {
/*
* If we fail to free the inode, shut down. The cancel
@@ -2352,7 +2326,7 @@ xfs_ifree(
struct xfs_inode_log_item *iip = ip->i_itemp;
int error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(VFS_I(ip)->i_nlink == 0);
ASSERT(ip->i_df.if_nextents == 0);
ASSERT(ip->i_disk_size == 0 || !S_ISREG(VFS_I(ip)->i_mode));
@@ -2421,7 +2395,7 @@ static void
xfs_iunpin(
struct xfs_inode *ip)
{
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
@@ -3182,7 +3156,7 @@ xfs_iflush(
struct xfs_mount *mp = ip->i_mount;
int error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED);
ASSERT(xfs_iflags_test(ip, XFS_IFLUSHING));
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_inode.h b/fs/xfs/xfs_inode.h
index 0c5bdb91152e..b70f3e8e4525 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -515,7 +515,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);
-bool xfs_isilocked(struct xfs_inode *, uint);
+void xfs_assert_ilocked(struct xfs_inode *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
uint xfs_ilock_attr_map_shared(struct xfs_inode *);
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 127b2410eb20..02427b53328f 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -648,7 +648,7 @@ xfs_inode_item_pin(
{
struct xfs_inode *ip = INODE_ITEM(lip)->ili_inode;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(lip->li_buf);
trace_xfs_inode_pin(ip, _RET_IP_);
@@ -754,7 +754,7 @@ xfs_inode_item_release(
unsigned short lock_flags;
ASSERT(ip->i_itemp != NULL);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
lock_flags = iip->ili_lock_flags;
iip->ili_lock_flags = 0;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 1c1e6171209d..2492b746912e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -791,8 +791,7 @@ xfs_setattr_size(
uint lock_flags = 0;
bool did_zeroing = false;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
- ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
ASSERT(S_ISREG(inode->i_mode));
ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
ATTR_MTIME_SET|ATTR_TIMES_SET)) == 0);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 086e78a6143a..660e7878db0d 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -254,7 +254,7 @@ xfs_qm_dqattach_one(
struct xfs_dquot *dqp;
int error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
error = 0;
/*
@@ -322,7 +322,7 @@ xfs_qm_dqattach_locked(
if (!xfs_qm_need_dqattach(ip))
return 0;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
@@ -353,7 +353,7 @@ xfs_qm_dqattach_locked(
* Don't worry about the dquots that we may have attached before any
* error - they'll get detached later if it has not already been done.
*/
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
return error;
}
@@ -1808,7 +1808,7 @@ xfs_qm_vop_chown(
XFS_TRANS_DQ_RTBCOUNT : XFS_TRANS_DQ_BCOUNT;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
ASSERT(XFS_IS_QUOTA_ON(ip->i_mount));
/* old dquot */
@@ -1896,7 +1896,7 @@ xfs_qm_vop_create_dqattach(
if (!XFS_IS_QUOTA_ON(mp))
return;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (udqp && XFS_IS_UQUOTA_ON(mp)) {
ASSERT(ip->i_udquot == NULL);
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index eb9102453aff..1267e008014e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -527,7 +527,7 @@ xfs_reflink_allocate_cow(
int error;
bool found;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (!ip->i_cowfp) {
ASSERT(!xfs_is_reflink_inode(ip));
xfs_ifork_init_cow(ip);
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 16534e9873f6..29382bc9d373 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1188,7 +1188,7 @@ xfs_rtallocate_extent(
xfs_fsblock_t sb; /* summary file block number */
struct xfs_buf *sumbp; /* summary file block buffer */
- ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
ASSERT(minlen > 0 && minlen <= maxlen);
/*
@@ -1431,7 +1431,7 @@ xfs_rtpick_extent(
uint64_t seq; /* sequence number of file creation */
uint64_t *seqp; /* pointer to seqno in inode */
- ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
seqp = (uint64_t *)&VFS_I(mp->m_rbmip)->i_atime;
if (!(mp->m_rbmip->i_diflags & XFS_DIFLAG_NEWRTBM)) {
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 85e433df6a3f..2ca157de4a73 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -43,7 +43,7 @@ xfs_readlink_bmap_ilocked(
int fsblocks = 0;
int offset;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
fsblocks = xfs_symlink_blocks(mp, pathlen);
error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index aa00cf67ad72..9c159d016ecf 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -796,7 +796,7 @@ xfs_trans_reserve_quota_nblks(
return 0;
ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+ xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
if (force)
qflags |= XFS_QMOPT_FORCE_RES;
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] xfs: Remove mrlock wrapper
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
` (2 preceding siblings ...)
2023-10-07 20:35 ` [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked Matthew Wilcox (Oracle)
@ 2023-10-07 20:35 ` Matthew Wilcox (Oracle)
2023-10-08 22:17 ` Dave Chinner
2023-10-07 20:35 ` [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl() Matthew Wilcox (Oracle)
2023-10-08 20:31 ` [PATCH v2 0/5] Remove the XFS mrlock Mateusz Guzik
5 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
mrlock was an rwsem wrapper that also recorded whether the lock was
held for read or write. Now that we can ask the generic code whether
the lock is held for read or write, we can remove this wrapper and use
an rwsem directly.
As the comment says, we can't use lockdep to assert that the ILOCK is
held for write, because we might be in a workqueue, and we aren't able
to tell lockdep that we do in fact own the lock.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/xfs/mrlock.h | 78 ----------------------------------------------
fs/xfs/xfs_inode.c | 22 +++++++------
fs/xfs/xfs_inode.h | 2 +-
fs/xfs/xfs_iops.c | 4 +--
fs/xfs/xfs_linux.h | 2 +-
fs/xfs/xfs_super.c | 4 +--
6 files changed, 18 insertions(+), 94 deletions(-)
delete mode 100644 fs/xfs/mrlock.h
diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
deleted file mode 100644
index 79155eec341b..000000000000
--- a/fs/xfs/mrlock.h
+++ /dev/null
@@ -1,78 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2000-2006 Silicon Graphics, Inc.
- * All Rights Reserved.
- */
-#ifndef __XFS_SUPPORT_MRLOCK_H__
-#define __XFS_SUPPORT_MRLOCK_H__
-
-#include <linux/rwsem.h>
-
-typedef struct {
- struct rw_semaphore mr_lock;
-#if defined(DEBUG) || defined(XFS_WARN)
- int mr_writer;
-#endif
-} mrlock_t;
-
-#if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(mrp, name) \
- do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
-#else
-#define mrinit(mrp, name) \
- do { init_rwsem(&(mrp)->mr_lock); } while (0)
-#endif
-
-#define mrlock_init(mrp, t,n,s) mrinit(mrp, n)
-#define mrfree(mrp) do { } while (0)
-
-static inline void mraccess_nested(mrlock_t *mrp, int subclass)
-{
- down_read_nested(&mrp->mr_lock, subclass);
-}
-
-static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
-{
- down_write_nested(&mrp->mr_lock, subclass);
-#if defined(DEBUG) || defined(XFS_WARN)
- mrp->mr_writer = 1;
-#endif
-}
-
-static inline int mrtryaccess(mrlock_t *mrp)
-{
- return down_read_trylock(&mrp->mr_lock);
-}
-
-static inline int mrtryupdate(mrlock_t *mrp)
-{
- if (!down_write_trylock(&mrp->mr_lock))
- return 0;
-#if defined(DEBUG) || defined(XFS_WARN)
- mrp->mr_writer = 1;
-#endif
- return 1;
-}
-
-static inline void mrunlock_excl(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
- mrp->mr_writer = 0;
-#endif
- up_write(&mrp->mr_lock);
-}
-
-static inline void mrunlock_shared(mrlock_t *mrp)
-{
- up_read(&mrp->mr_lock);
-}
-
-static inline void mrdemote(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
- mrp->mr_writer = 0;
-#endif
- downgrade_write(&mrp->mr_lock);
-}
-
-#endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 812d6f255d84..a6d9a834d869 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -208,9 +208,9 @@ xfs_ilock(
}
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));
}
/*
@@ -251,10 +251,10 @@ xfs_ilock_nowait(
}
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;
@@ -303,9 +303,9 @@ xfs_iunlock(
up_read(&VFS_I(ip)->i_mapping->invalidate_lock);
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_);
}
@@ -324,7 +324,7 @@ 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)
downgrade_write(&VFS_I(ip)->i_mapping->invalidate_lock);
if (lock_flags & XFS_IOLOCK_EXCL)
@@ -338,10 +338,14 @@ xfs_assert_ilocked(
struct xfs_inode *ip,
uint lock_flags)
{
+ /*
+ * Sometimes we assert the ILOCK is held exclusively, but we're in
+ * a workqueue, so lockdep doesn't know we're the owner.
+ */
if (lock_flags & XFS_ILOCK_SHARED)
- rwsem_assert_held(&ip->i_lock.mr_lock);
+ rwsem_assert_held(&ip->i_lock);
else if (lock_flags & XFS_ILOCK_EXCL)
- BUG_ON(!ip->i_lock.mr_writer);
+ __rwsem_assert_held_write(&ip->i_lock);
if (lock_flags & XFS_MMAPLOCK_SHARED)
rwsem_assert_held(&VFS_I(ip)->i_mapping->invalidate_lock);
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index b70f3e8e4525..1c2484b6afff 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -39,7 +39,7 @@ typedef struct xfs_inode {
/* Transaction and locking information. */
struct xfs_inode_log_item *i_itemp; /* logging information */
- mrlock_t i_lock; /* inode lock */
+ struct rw_semaphore i_lock; /* inode lock */
atomic_t i_pincount; /* inode pin count */
struct llist_node i_gclist; /* deferred inactivation list */
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 2492b746912e..fcceb2b4e40d 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1279,9 +1279,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 e9d317a3dafe..15fdaef578fe 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>
@@ -51,6 +50,7 @@ typedef __u32 xfs_nlink_t;
#include <linux/notifier.h>
#include <linux/delay.h>
#include <linux/log2.h>
+#include <linux/rwsem.h>
#include <linux/spinlock.h>
#include <linux/random.h>
#include <linux/ctype.h>
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 819a3568b28f..19435fa4c6b3 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -717,9 +717,7 @@ xfs_fs_inode_init_once(
/* xfs inode */
atomic_set(&ip->i_pincount, 0);
spin_lock_init(&ip->i_flags_lock);
-
- mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
- "xfsino", ip->i_ino);
+ init_rwsem(&ip->i_lock);
}
/*
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl()
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
` (3 preceding siblings ...)
2023-10-07 20:35 ` [PATCH v2 4/5] xfs: Remove mrlock wrapper Matthew Wilcox (Oracle)
@ 2023-10-07 20:35 ` Matthew Wilcox (Oracle)
2023-10-08 20:26 ` Mateusz Guzik
2023-10-08 20:31 ` [PATCH v2 0/5] Remove the XFS mrlock Mateusz Guzik
5 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox (Oracle) @ 2023-10-07 20:35 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long
Cc: Matthew Wilcox (Oracle),
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
Use the new rwsem_assert_held functions to implement these new
assertions. Convert the inode_is_locked() callers in the VFS to
use them.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/attr.c | 2 +-
fs/namei.c | 6 +++---
include/linux/fs.h | 10 ++++++++++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index a8ae5f6d9b16..5e32b0a4f8c2 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -387,7 +387,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
struct timespec64 now;
unsigned int ia_valid = attr->ia_valid;
- WARN_ON_ONCE(!inode_is_locked(inode));
+ inode_assert_locked_excl(inode);
error = may_setattr(idmap, inode, ia_valid);
if (error)
diff --git a/fs/namei.c b/fs/namei.c
index 567ee547492b..6b595ad4318d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2708,7 +2708,7 @@ struct dentry *try_lookup_one_len(const char *name, struct dentry *base, int len
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
if (err)
@@ -2735,7 +2735,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
if (err)
@@ -2765,7 +2765,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(idmap, name, base, len, &this);
if (err)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b528f063e8ff..e01e041c102b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -832,6 +832,16 @@ static inline int inode_is_locked(struct inode *inode)
return rwsem_is_locked(&inode->i_rwsem);
}
+static inline void inode_assert_locked(const struct inode *inode)
+{
+ rwsem_assert_held(&inode->i_rwsem);
+}
+
+static inline void inode_assert_locked_excl(const struct inode *inode)
+{
+ rwsem_assert_held_write(&inode->i_rwsem);
+}
+
static inline void inode_lock_nested(struct inode *inode, unsigned subclass)
{
down_write_nested(&inode->i_rwsem, subclass);
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl()
2023-10-07 20:35 ` [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl() Matthew Wilcox (Oracle)
@ 2023-10-08 20:26 ` Mateusz Guzik
2023-10-08 21:05 ` Matthew Wilcox
0 siblings, 1 reply; 14+ messages in thread
From: Mateusz Guzik @ 2023-10-08 20:26 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs
On 10/7/23, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> +static inline void inode_assert_locked_excl(const struct inode *inode)
> +{
> + rwsem_assert_held_write(&inode->i_rwsem);
> +}
> +
> static inline void inode_lock_nested(struct inode *inode, unsigned
> subclass)
> {
> down_write_nested(&inode->i_rwsem, subclass);
Why "excl" instead of "write"? Apart from looking weird, it is
inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/5] Remove the XFS mrlock
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
` (4 preceding siblings ...)
2023-10-07 20:35 ` [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl() Matthew Wilcox (Oracle)
@ 2023-10-08 20:31 ` Mateusz Guzik
5 siblings, 0 replies; 14+ messages in thread
From: Mateusz Guzik @ 2023-10-08 20:31 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs
On 10/7/23, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> I'm sympathetic to "this will warn twice and dump much the same
> information if you have lockdep enabled". Perhaps somebody has a
> suggestion for not doing that?
>
Well the obvious idea is that lockdep could provide a macro indicating
what's up.
Then you would:
static inline void rwsem_assert_held(const struct rw_semaphore *sem)
{
if (lockdep_works)
lockdep_assert_held(sem);
else
__rwsem_assert_held(sem);
}
Am I missing something? If this is not feasible to achieve, then the
proposed routines need a comment justifying the state.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl()
2023-10-08 20:26 ` Mateusz Guzik
@ 2023-10-08 21:05 ` Matthew Wilcox
2023-10-08 21:21 ` Mateusz Guzik
0 siblings, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2023-10-08 21:05 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs
On Sun, Oct 08, 2023 at 10:26:40PM +0200, Mateusz Guzik wrote:
> On 10/7/23, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> > +static inline void inode_assert_locked_excl(const struct inode *inode)
> > +{
> > + rwsem_assert_held_write(&inode->i_rwsem);
> > +}
> > +
> > static inline void inode_lock_nested(struct inode *inode, unsigned
> > subclass)
> > {
> > down_write_nested(&inode->i_rwsem, subclass);
>
> Why "excl" instead of "write"? Apart from looking weird, it is
> inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
Yes, but that pairs with i_mmap_lock_write() / i_mmap_lock_read().
The problem is that we have inode_lock() / inode_lock_shared()
inode_assert_locked_read/write doesn't make sense with them. But
inode_assert_locked() doesn't make sense as the assertion for
inode_lock() because you'd expect it to assert whether the inode lock
is held at all. So I went with inode_assert_locked_excl().
I wouldn't mind if we converted all the inode_lock()/shared to
inode_lock_read() / inode_lock_write(), and then added
inode_assert_read_locked() / inode_assert_write_locked(). That's
a bit of a bigger job than I want to take on today.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl()
2023-10-08 21:05 ` Matthew Wilcox
@ 2023-10-08 21:21 ` Mateusz Guzik
0 siblings, 0 replies; 14+ messages in thread
From: Mateusz Guzik @ 2023-10-08 21:21 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, linux-fsdevel, Christian Brauner
On 10/8/23, Matthew Wilcox <willy@infradead.org> wrote:
> On Sun, Oct 08, 2023 at 10:26:40PM +0200, Mateusz Guzik wrote:
>> On 10/7/23, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
>> > +static inline void inode_assert_locked_excl(const struct inode *inode)
>> > +{
>> > + rwsem_assert_held_write(&inode->i_rwsem);
>> > +}
>> > +
>> > static inline void inode_lock_nested(struct inode *inode, unsigned
>> > subclass)
>> > {
>> > down_write_nested(&inode->i_rwsem, subclass);
>>
>> Why "excl" instead of "write"? Apart from looking weird, it is
>> inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
>
> Yes, but that pairs with i_mmap_lock_write() / i_mmap_lock_read().
>
> The problem is that we have inode_lock() / inode_lock_shared()
> inode_assert_locked_read/write doesn't make sense with them. But
> inode_assert_locked() doesn't make sense as the assertion for
> inode_lock() because you'd expect it to assert whether the inode lock
> is held at all. So I went with inode_assert_locked_excl().
>
> I wouldn't mind if we converted all the inode_lock()/shared to
> inode_lock_read() / inode_lock_write(), and then added
> inode_assert_read_locked() / inode_assert_write_locked(). That's
> a bit of a bigger job than I want to take on today.
>
I agree it is rather messy and I'm not going to spend time arguing as
it is not my call anyway.
Speaking of that, I just noticed the vfs folk are not CC'ed, which I'm
rectifying with this e-mail.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write()
2023-10-07 20:35 ` [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write() Matthew Wilcox (Oracle)
@ 2023-10-08 21:54 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2023-10-08 21:54 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
On Sat, Oct 07, 2023 at 09:35:39PM +0100, Matthew Wilcox (Oracle) wrote:
> Modelled after lockdep_assert_held() and lockdep_assert_held_write(),
> but are always active, even when lockdep is disabled. Of course, they
> don't test that _this_ thread is the owner, but it's sufficient to catch
> many bugs and doesn't incur the same performance penalty as lockdep.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
.....
> @@ -169,6 +189,18 @@ static __always_inline int rwsem_is_contended(struct rw_semaphore *sem)
> * the RT specific variant.
> */
>
> +static inline void rwsem_assert_held(const struct rw_semaphore *sem)
> +{
> + lockdep_assert_held(sem);
> + __rwsem_assert_held(sem);
> +}
if (IS_ENABLED(CONFIG_LOCKDEP))
lockdep_assert_held(sem);
else
__rwsem_assert_held(sem);
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked
2023-10-07 20:35 ` [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked Matthew Wilcox (Oracle)
@ 2023-10-08 21:59 ` Dave Chinner
2023-10-09 18:27 ` Darrick J. Wong
1 sibling, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2023-10-08 21:59 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
On Sat, Oct 07, 2023 at 09:35:41PM +0100, Matthew Wilcox (Oracle) wrote:
> To use the new rwsem_assert_held()/rwsem_assert_held_write(), we can't
> use the existing ASSERT macro.
>
> xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL) checks both the
> IOLOCK and the ILOCK are held for write. xfs_isilocked() only checked
> that the ILOCK was held for write.
>
> xfs_assert_ilocked() is always on, even if DEBUG or XFS_WARN aren't
> defined. It's a cheap check, so I don't think it's worth defining
> it away.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
......
> -bool
> -xfs_isilocked(
> +void
> +xfs_assert_ilocked(
> 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_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> - return __xfs_rwsem_islocked(&VFS_I(ip)->i_mapping->invalidate_lock,
> - (lock_flags & XFS_MMAPLOCK_SHARED));
> - }
> -
> - if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> - return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
> - (lock_flags & XFS_IOLOCK_SHARED));
> - }
> -
> - ASSERT(0);
> - return false;
> + if (lock_flags & XFS_ILOCK_SHARED)
> + rwsem_assert_held(&ip->i_lock.mr_lock);
> + else if (lock_flags & XFS_ILOCK_EXCL)
> + BUG_ON(!ip->i_lock.mr_writer);
ASSERT(!ip->i_lock.mr_writer);
Otherwise OK.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/5] xfs: Remove mrlock wrapper
2023-10-07 20:35 ` [PATCH v2 4/5] xfs: Remove mrlock wrapper Matthew Wilcox (Oracle)
@ 2023-10-08 22:17 ` Dave Chinner
0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2023-10-08 22:17 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, Darrick J . Wong,
linux-xfs, Mateusz Guzik
On Sat, Oct 07, 2023 at 09:35:42PM +0100, Matthew Wilcox (Oracle) wrote:
> mrlock was an rwsem wrapper that also recorded whether the lock was
> held for read or write. Now that we can ask the generic code whether
> the lock is held for read or write, we can remove this wrapper and use
> an rwsem directly.
>
> As the comment says, we can't use lockdep to assert that the ILOCK is
> held for write, because we might be in a workqueue, and we aren't able
> to tell lockdep that we do in fact own the lock.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
.....
> @@ -338,10 +338,14 @@ xfs_assert_ilocked(
> struct xfs_inode *ip,
> uint lock_flags)
> {
> + /*
> + * Sometimes we assert the ILOCK is held exclusively, but we're in
> + * a workqueue, so lockdep doesn't know we're the owner.
> + */
> if (lock_flags & XFS_ILOCK_SHARED)
> - rwsem_assert_held(&ip->i_lock.mr_lock);
> + rwsem_assert_held(&ip->i_lock);
> else if (lock_flags & XFS_ILOCK_EXCL)
> - BUG_ON(!ip->i_lock.mr_writer);
> + __rwsem_assert_held_write(&ip->i_lock);
It took me ages to work out that the comment related to the "__"
variant of rwsem_assert_held_write() function. I really dislike the
use of "__" prefixes for a function with slightly different,
non-obvious semantics to the parent - it's way too subtle for it to
be clear that this is what the comment is refering to.
In this case, we effectively have rwsem_assert_held_write() that
does lockdep checks, and __rwsem_assert_held_write() that does not
do lockdep checks. Either the former needs to say "lockdep" or the
latter needs "nolockdep" in the name to indicate to the reader the
intent of the code calling the checking function....
Otherwise the code looks fine.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked
2023-10-07 20:35 ` [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked Matthew Wilcox (Oracle)
2023-10-08 21:59 ` Dave Chinner
@ 2023-10-09 18:27 ` Darrick J. Wong
1 sibling, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2023-10-09 18:27 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
linux-kernel, linux-mm, Chandan Babu R, linux-xfs, Mateusz Guzik
On Sat, Oct 07, 2023 at 09:35:41PM +0100, Matthew Wilcox (Oracle) wrote:
> To use the new rwsem_assert_held()/rwsem_assert_held_write(), we can't
> use the existing ASSERT macro.
>
> xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL) checks both the
> IOLOCK and the ILOCK are held for write. xfs_isilocked() only checked
> that the ILOCK was held for write.
>
> xfs_assert_ilocked() is always on, even if DEBUG or XFS_WARN aren't
> defined. It's a cheap check, so I don't think it's worth defining
> it away.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/xfs/libxfs/xfs_attr.c | 2 +-
> fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
> fs/xfs/libxfs/xfs_bmap.c | 19 +++++----
> fs/xfs/libxfs/xfs_defer.c | 2 +-
> fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
> fs/xfs/libxfs/xfs_rtbitmap.c | 2 +-
> fs/xfs/libxfs/xfs_trans_inode.c | 6 +--
> fs/xfs/scrub/readdir.c | 4 +-
> fs/xfs/xfs_attr_list.c | 2 +-
> fs/xfs/xfs_bmap_util.c | 10 ++---
> fs/xfs/xfs_dir2_readdir.c | 2 +-
> fs/xfs/xfs_dquot.c | 4 +-
> fs/xfs/xfs_file.c | 4 +-
> fs/xfs/xfs_inode.c | 72 +++++++++++----------------------
> fs/xfs/xfs_inode.h | 2 +-
> fs/xfs/xfs_inode_item.c | 4 +-
> fs/xfs/xfs_iops.c | 3 +-
> fs/xfs/xfs_qm.c | 10 ++---
> fs/xfs/xfs_reflink.c | 2 +-
> fs/xfs/xfs_rtalloc.c | 4 +-
> fs/xfs/xfs_symlink.c | 2 +-
> fs/xfs/xfs_trans_dquot.c | 2 +-
> 22 files changed, 66 insertions(+), 96 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e28d93d232de..ebf0722d8963 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -224,7 +224,7 @@ int
> xfs_attr_get_ilocked(
> struct xfs_da_args *args)
> {
> - ASSERT(xfs_isilocked(args->dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(args->dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
Bikeshed: I might've gone with xfs_assert_ilocked(), especially
since that's what the commit message says. ;)
--D
>
> if (!xfs_inode_hasattr(args->dp))
> return -ENOATTR;
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index d440393b40eb..1c007ebf153a 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -545,7 +545,7 @@ xfs_attr_rmtval_stale(
> struct xfs_buf *bp;
> int error;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> if (XFS_IS_CORRUPT(mp, map->br_startblock == DELAYSTARTBLOCK) ||
> XFS_IS_CORRUPT(mp, map->br_startblock == HOLESTARTBLOCK))
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 30c931b38853..d70cf543a52d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1189,7 +1189,7 @@ xfs_iread_extents(
> if (!xfs_need_iread_extents(ifp))
> return 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> ir.loaded = 0;
> xfs_iext_first(ifp, &ir.icur);
> @@ -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));
> + xfs_assert_ilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL);
>
> if (WARN_ON_ONCE(!ifp))
> return -EFSCORRUPTED;
> @@ -4354,7 +4354,7 @@ xfs_bmapi_write(
> ASSERT(tp != NULL);
> ASSERT(len > 0);
> ASSERT(ifp->if_format != XFS_DINODE_FMT_LOCAL);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(!(flags & XFS_BMAPI_REMAP));
>
> /* zeroing is for currently only for data extents, not metadata */
> @@ -4651,7 +4651,7 @@ xfs_bmapi_remap(
> ifp = xfs_ifork_ptr(ip, whichfork);
> ASSERT(len > 0);
> ASSERT(len <= (xfs_filblks_t)XFS_MAX_BMBT_EXTLEN);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
> XFS_BMAPI_NORMAP)));
> ASSERT((flags & (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC)) !=
> @@ -5305,7 +5305,7 @@ __xfs_bunmapi(
> if (xfs_is_shutdown(mp))
> return -EIO;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(len > 0);
> ASSERT(nexts >= 0);
>
> @@ -5648,8 +5648,7 @@ xfs_bmse_merge(
>
> blockcount = left->br_blockcount + got->br_blockcount;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
> ASSERT(xfs_bmse_can_merge(left, got, shift));
>
> new = *left;
> @@ -5777,7 +5776,7 @@ xfs_bmap_collapse_extents(
> if (xfs_is_shutdown(mp))
> return -EIO;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
>
> error = xfs_iread_extents(tp, ip, whichfork);
> if (error)
> @@ -5850,7 +5849,7 @@ xfs_bmap_can_insert_extents(
> int is_empty;
> int error = 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
>
> if (xfs_is_shutdown(ip->i_mount))
> return -EIO;
> @@ -5892,7 +5891,7 @@ xfs_bmap_insert_extents(
> if (xfs_is_shutdown(mp))
> return -EIO;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);
>
> error = xfs_iread_extents(tp, ip, whichfork);
> if (error)
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index bcfb6a4203cd..7927a721dc86 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -744,7 +744,7 @@ xfs_defer_ops_capture(
> * transaction.
> */
> for (i = 0; i < dfc->dfc_held.dr_inos; i++) {
> - ASSERT(xfs_isilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(dfc->dfc_held.dr_ip[i], XFS_ILOCK_EXCL);
> ihold(VFS_I(dfc->dfc_held.dr_ip[i]));
> }
>
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 5a2e7ddfa76d..14193e044f3d 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -563,7 +563,7 @@ xfs_iextents_copy(
> struct xfs_bmbt_irec rec;
> int64_t copied = 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
> ASSERT(ifp->if_bytes > 0);
>
> for_each_xfs_iext(ifp, &icur, &rec) {
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index fa180ab66b73..4146b8697c89 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -974,7 +974,7 @@ xfs_rtfree_extent(
> mp = tp->t_mountp;
>
> ASSERT(mp->m_rbmip->i_itemp != NULL);
> - ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
>
> error = xfs_rtcheck_alloc_range(mp, tp, bno, len);
> if (error)
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 6b2296ff248a..05229fdef36b 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -31,7 +31,7 @@ xfs_trans_ijoin(
> {
> struct xfs_inode_log_item *iip;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> if (ip->i_itemp == NULL)
> xfs_inode_item_init(ip, ip->i_mount);
> iip = ip->i_itemp;
> @@ -60,7 +60,7 @@ xfs_trans_ichgtime(
> struct timespec64 tv;
>
> ASSERT(tp);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> tv = current_time(inode);
>
> @@ -90,7 +90,7 @@ xfs_trans_log_inode(
> struct inode *inode = VFS_I(ip);
>
> ASSERT(iip);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(!xfs_iflags_test(ip, XFS_ISTALE));
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> diff --git a/fs/xfs/scrub/readdir.c b/fs/xfs/scrub/readdir.c
> index e51c1544be63..0b200bab04c8 100644
> --- a/fs/xfs/scrub/readdir.c
> +++ b/fs/xfs/scrub/readdir.c
> @@ -283,7 +283,7 @@ xchk_dir_walk(
> return -EIO;
>
> ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
> - ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
>
> if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> return xchk_dir_walk_sf(sc, dp, dirent_fn, priv);
> @@ -334,7 +334,7 @@ xchk_dir_lookup(
> return -EIO;
>
> ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
> - ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
>
> if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL) {
> error = xfs_dir2_sf_lookup(&args);
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 99bbbe1a0e44..235dd125c92f 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -505,7 +505,7 @@ xfs_attr_list_ilocked(
> {
> struct xfs_inode *dp = context->dp;
>
> - ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
>
> /*
> * Decide on what work routines to call based on the inode size.
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fcefab687285..1bd430f69d44 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -655,8 +655,8 @@ xfs_can_free_eofblocks(
> * Caller must either hold the exclusive io lock; or be inactivating
> * the inode, which guarantees there are no other users of the inode.
> */
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL) ||
> - (VFS_I(ip)->i_state & I_FREEING));
> + if (!(VFS_I(ip)->i_state & I_FREEING))
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
>
> /* prealloc/delalloc exists only on regular files */
> if (!S_ISREG(VFS_I(ip)->i_mode))
> @@ -1112,8 +1112,7 @@ xfs_collapse_file_space(
> xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
> bool done = false;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
>
> trace_xfs_collapse_file_space(ip);
>
> @@ -1182,8 +1181,7 @@ xfs_insert_file_space(
> xfs_fileoff_t shift_fsb = XFS_B_TO_FSB(mp, len);
> bool done = false;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
>
> trace_xfs_insert_file_space(ip);
>
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 9f3ceb461515..95cd8b9cf3dc 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -521,7 +521,7 @@ xfs_readdir(
> return -EIO;
>
> ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
> - ASSERT(xfs_isilocked(dp, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
> + xfs_assert_ilocked(dp, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL);
> XFS_STATS_INC(dp->i_mount, xs_dir_getdents);
>
> args.dp = dp;
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index ac6ba646624d..4b2f1b82badc 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -949,7 +949,7 @@ xfs_qm_dqget_inode(
> if (error)
> return error;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(xfs_inode_dquot(ip, type) == NULL);
>
> id = xfs_qm_id_for_quotatype(ip, type);
> @@ -1006,7 +1006,7 @@ xfs_qm_dqget_inode(
> }
>
> dqret:
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> trace_xfs_dqget_miss(dqp);
> *O_dqpp = dqp;
> return 0;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 203700278ddb..9ba2b89b3862 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -842,7 +842,7 @@ xfs_break_dax_layouts(
> {
> struct page *page;
>
> - ASSERT(xfs_isilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL));
> + xfs_assert_ilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL);
>
> page = dax_layout_busy_page(inode->i_mapping);
> if (!page)
> @@ -863,7 +863,7 @@ xfs_break_layouts(
> bool retry;
> int error;
>
> - ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
> + xfs_assert_ilocked(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 4d55f58d99b7..812d6f255d84 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -333,52 +333,26 @@ xfs_ilock_demote(
> trace_xfs_ilock_demote(ip, lock_flags, _RET_IP_);
> }
>
> -#if defined(DEBUG) || defined(XFS_WARN)
> -static inline bool
> -__xfs_rwsem_islocked(
> - struct rw_semaphore *rwsem,
> - bool shared)
> -{
> - if (!debug_locks)
> - return rwsem_is_locked(rwsem);
> -
> - if (!shared)
> - return lockdep_is_held_type(rwsem, 0);
> -
> - /*
> - * We are checking that the lock is held at least in shared
> - * mode but don't care that it might be held exclusively
> - * (i.e. shared | excl). Hence we check if the lock is held
> - * in any mode rather than an explicit shared mode.
> - */
> - return lockdep_is_held_type(rwsem, -1);
> -}
> -
> -bool
> -xfs_isilocked(
> +void
> +xfs_assert_ilocked(
> 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_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> - return __xfs_rwsem_islocked(&VFS_I(ip)->i_mapping->invalidate_lock,
> - (lock_flags & XFS_MMAPLOCK_SHARED));
> - }
> -
> - if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> - return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
> - (lock_flags & XFS_IOLOCK_SHARED));
> - }
> -
> - ASSERT(0);
> - return false;
> + if (lock_flags & XFS_ILOCK_SHARED)
> + rwsem_assert_held(&ip->i_lock.mr_lock);
> + else if (lock_flags & XFS_ILOCK_EXCL)
> + BUG_ON(!ip->i_lock.mr_writer);
> +
> + if (lock_flags & XFS_MMAPLOCK_SHARED)
> + rwsem_assert_held(&VFS_I(ip)->i_mapping->invalidate_lock);
> + else if (lock_flags & XFS_MMAPLOCK_EXCL)
> + rwsem_assert_held_write(&VFS_I(ip)->i_mapping->invalidate_lock);
> +
> + if (lock_flags & XFS_IOLOCK_SHARED)
> + rwsem_assert_held(&VFS_I(ip)->i_rwsem);
> + else if (lock_flags & XFS_IOLOCK_EXCL)
> + rwsem_assert_held_write(&VFS_I(ip)->i_rwsem);
> }
> -#endif
>
> /*
> * xfs_lockdep_subclass_ok() is only used in an ASSERT, so is only called when
> @@ -1335,9 +1309,9 @@ xfs_itruncate_extents_flags(
> xfs_filblks_t unmap_len;
> int error = 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> - ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
> - xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> + if (atomic_read(&VFS_I(ip)->i_count))
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL);
> ASSERT(new_size <= XFS_ISIZE(ip));
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> ASSERT(ip->i_itemp != NULL);
> @@ -1598,7 +1572,7 @@ xfs_inactive_ifree(
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>
> error = xfs_ifree(tp, ip);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> if (error) {
> /*
> * If we fail to free the inode, shut down. The cancel
> @@ -2352,7 +2326,7 @@ xfs_ifree(
> struct xfs_inode_log_item *iip = ip->i_itemp;
> int error;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(VFS_I(ip)->i_nlink == 0);
> ASSERT(ip->i_df.if_nextents == 0);
> ASSERT(ip->i_disk_size == 0 || !S_ISREG(VFS_I(ip)->i_mode));
> @@ -2421,7 +2395,7 @@ static void
> xfs_iunpin(
> struct xfs_inode *ip)
> {
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED);
>
> trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
>
> @@ -3182,7 +3156,7 @@ xfs_iflush(
> struct xfs_mount *mp = ip->i_mount;
> int error;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED);
> ASSERT(xfs_iflags_test(ip, XFS_IFLUSHING));
> 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_inode.h b/fs/xfs/xfs_inode.h
> index 0c5bdb91152e..b70f3e8e4525 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -515,7 +515,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);
> -bool xfs_isilocked(struct xfs_inode *, uint);
> +void xfs_assert_ilocked(struct xfs_inode *, uint);
> uint xfs_ilock_data_map_shared(struct xfs_inode *);
> uint xfs_ilock_attr_map_shared(struct xfs_inode *);
>
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 127b2410eb20..02427b53328f 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -648,7 +648,7 @@ xfs_inode_item_pin(
> {
> struct xfs_inode *ip = INODE_ITEM(lip)->ili_inode;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(lip->li_buf);
>
> trace_xfs_inode_pin(ip, _RET_IP_);
> @@ -754,7 +754,7 @@ xfs_inode_item_release(
> unsigned short lock_flags;
>
> ASSERT(ip->i_itemp != NULL);
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> lock_flags = iip->ili_lock_flags;
> iip->ili_lock_flags = 0;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1c1e6171209d..2492b746912e 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -791,8 +791,7 @@ xfs_setattr_size(
> uint lock_flags = 0;
> bool did_zeroing = false;
>
> - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> - ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
> ASSERT(S_ISREG(inode->i_mode));
> ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> ATTR_MTIME_SET|ATTR_TIMES_SET)) == 0);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 086e78a6143a..660e7878db0d 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -254,7 +254,7 @@ xfs_qm_dqattach_one(
> struct xfs_dquot *dqp;
> int error;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> error = 0;
>
> /*
> @@ -322,7 +322,7 @@ xfs_qm_dqattach_locked(
> if (!xfs_qm_need_dqattach(ip))
> return 0;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
> @@ -353,7 +353,7 @@ xfs_qm_dqattach_locked(
> * Don't worry about the dquots that we may have attached before any
> * error - they'll get detached later if it has not already been done.
> */
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> return error;
> }
>
> @@ -1808,7 +1808,7 @@ xfs_qm_vop_chown(
> XFS_TRANS_DQ_RTBCOUNT : XFS_TRANS_DQ_BCOUNT;
>
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> ASSERT(XFS_IS_QUOTA_ON(ip->i_mount));
>
> /* old dquot */
> @@ -1896,7 +1896,7 @@ xfs_qm_vop_create_dqattach(
> if (!XFS_IS_QUOTA_ON(mp))
> return;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index eb9102453aff..1267e008014e 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -527,7 +527,7 @@ xfs_reflink_allocate_cow(
> int error;
> bool found;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> if (!ip->i_cowfp) {
> ASSERT(!xfs_is_reflink_inode(ip));
> xfs_ifork_init_cow(ip);
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 16534e9873f6..29382bc9d373 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1188,7 +1188,7 @@ xfs_rtallocate_extent(
> xfs_fsblock_t sb; /* summary file block number */
> struct xfs_buf *sumbp; /* summary file block buffer */
>
> - ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
> ASSERT(minlen > 0 && minlen <= maxlen);
>
> /*
> @@ -1431,7 +1431,7 @@ xfs_rtpick_extent(
> uint64_t seq; /* sequence number of file creation */
> uint64_t *seqp; /* pointer to seqno in inode */
>
> - ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL);
>
> seqp = (uint64_t *)&VFS_I(mp->m_rbmip)->i_atime;
> if (!(mp->m_rbmip->i_diflags & XFS_DIFLAG_NEWRTBM)) {
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 85e433df6a3f..2ca157de4a73 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -43,7 +43,7 @@ xfs_readlink_bmap_ilocked(
> int fsblocks = 0;
> int offset;
>
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
>
> fsblocks = xfs_symlink_blocks(mp, pathlen);
> error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index aa00cf67ad72..9c159d016ecf 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -796,7 +796,7 @@ xfs_trans_reserve_quota_nblks(
> return 0;
>
> ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> + xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>
> if (force)
> qflags |= XFS_QMOPT_FORCE_RES;
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-10-09 18:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-07 20:35 [PATCH v2 0/5] Remove the XFS mrlock Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 1/5] locking: Add rwsem_assert_held() and rwsem_assert_held_write() Matthew Wilcox (Oracle)
2023-10-08 21:54 ` Dave Chinner
2023-10-07 20:35 ` [PATCH v2 2/5] mm: Use rwsem assertion macros for mmap_lock Matthew Wilcox (Oracle)
2023-10-07 20:35 ` [PATCH v2 3/5] xfs: Replace xfs_isilocked with xfs_assert_locked Matthew Wilcox (Oracle)
2023-10-08 21:59 ` Dave Chinner
2023-10-09 18:27 ` Darrick J. Wong
2023-10-07 20:35 ` [PATCH v2 4/5] xfs: Remove mrlock wrapper Matthew Wilcox (Oracle)
2023-10-08 22:17 ` Dave Chinner
2023-10-07 20:35 ` [PATCH v2 5/5] fs: Add inode_assert_locked() and inode_assert_locked_excl() Matthew Wilcox (Oracle)
2023-10-08 20:26 ` Mateusz Guzik
2023-10-08 21:05 ` Matthew Wilcox
2023-10-08 21:21 ` Mateusz Guzik
2023-10-08 20:31 ` [PATCH v2 0/5] Remove the XFS mrlock Mateusz Guzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).