* [PATCH v4 2/4] xfs: Fix WS in xfs_isilocked() calls
2020-02-11 22:10 [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
@ 2020-02-11 22:10 ` Pavel Reichl
2020-02-12 0:35 ` Dave Chinner
2020-02-11 22:10 ` [PATCH v4 3/4] xfs: Fix bug when checking diff. locks Pavel Reichl
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Pavel Reichl @ 2020-02-11 22:10 UTC (permalink / raw)
To: linux-xfs
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
fs/xfs/libxfs/xfs_bmap.c | 2 +-
fs/xfs/xfs_file.c | 3 ++-
fs/xfs/xfs_inode.c | 6 +++---
fs/xfs/xfs_qm.c | 2 +-
4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9a6d7a84689a..bc2be29193aa 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3906,7 +3906,7 @@ xfs_bmapi_read(
ASSERT(*nmap >= 1);
ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
XFS_BMAPI_COWFORK)));
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b8a4a3f29b36..6b65572bdb21 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -770,7 +770,8 @@ xfs_break_layouts(
bool retry;
int error;
- ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
+ ASSERT(xfs_isilocked(XFS_I(inode),
+ XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
do {
retry = false;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index cfefa7543b37..be1f06fe1016 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2831,7 +2831,7 @@ static void
xfs_iunpin(
struct xfs_inode *ip)
{
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
@@ -3695,7 +3695,7 @@ xfs_iflush(
XFS_STATS_INC(mp, xs_iflush_count);
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
ASSERT(xfs_isiflocked(ip));
ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
@@ -3827,7 +3827,7 @@ xfs_iflush_int(
struct xfs_dinode *dip;
struct xfs_mount *mp = ip->i_mount;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
ASSERT(xfs_isiflocked(ip));
ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 0b0909657bad..d2896925b5ca 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1802,7 +1802,7 @@ xfs_qm_vop_chown_reserve(
int error;
- ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
ASSERT(XFS_IS_QUOTA_RUNNING(mp));
delblks = ip->i_delayed_blks;
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 2/4] xfs: Fix WS in xfs_isilocked() calls
2020-02-11 22:10 ` [PATCH v4 2/4] xfs: Fix WS in xfs_isilocked() calls Pavel Reichl
@ 2020-02-12 0:35 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2020-02-12 0:35 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
On Tue, Feb 11, 2020 at 11:10:16PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 2 +-
> fs/xfs/xfs_file.c | 3 ++-
> fs/xfs/xfs_inode.c | 6 +++---
> fs/xfs/xfs_qm.c | 2 +-
> 4 files changed, 7 insertions(+), 6 deletions(-)
Hi Pavel,
I don't know what this patch does from reading the subject line,
and the commit message doesn't tell me, either. :)
IOWs, please don't use abbreviations in the subject line because not
everyone understands what the abbreviation you used means. This
makes it hard to read the git history once the patch is committed.
Similarly, subjects that say "Fix the frobnozzle" can be better
written as something like:
xfs: clean up whitespace in xfs_isilocked() calls
So that the output of 'git log --oneline fs/xfs' it's totally
obvious what the commit is doing from the summary. "Fix the
frobnozzle" is ambiguous because "fix" can mean many things, hence
to find out you need to look at the specific commit in more detail
and that slows down code and history searches....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 3/4] xfs: Fix bug when checking diff. locks
2020-02-11 22:10 [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
2020-02-11 22:10 ` [PATCH v4 2/4] xfs: Fix WS in xfs_isilocked() calls Pavel Reichl
@ 2020-02-11 22:10 ` Pavel Reichl
2020-02-12 0:38 ` Dave Chinner
2020-02-11 22:10 ` [PATCH v4 4/4] xfs: Replace mrlock_t by rw_semaphore Pavel Reichl
2020-02-11 22:27 ` [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Eric Sandeen
3 siblings, 1 reply; 8+ messages in thread
From: Pavel Reichl @ 2020-02-11 22:10 UTC (permalink / raw)
To: linux-xfs
In its current form, xfs_isilocked() is only able to test one lock type at a
time - ilock, iolock, or mmap lock, but combinations are not properly handled.
The intent here is to check that both XFS_IOLOCK_EXCL and XFS_ILOCK_EXCL are
held, so test them each separately.
The commit ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") ORed the flags
together which was an error, so this patch reverts that part of the change and
check the locks independently.
Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
Changelog from V3:
Commit message extened.
fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index bc2be29193aa..c9dc94f114ed 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5829,7 +5829,8 @@ xfs_bmap_collapse_extents(
if (XFS_FORCED_SHUTDOWN(mp))
return -EIO;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (!(ifp->if_flags & XFS_IFEXTENTS)) {
error = xfs_iread_extents(tp, ip, whichfork);
@@ -5946,7 +5947,8 @@ xfs_bmap_insert_extents(
if (XFS_FORCED_SHUTDOWN(mp))
return -EIO;
- ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+ ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (!(ifp->if_flags & XFS_IFEXTENTS)) {
error = xfs_iread_extents(tp, ip, whichfork);
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 3/4] xfs: Fix bug when checking diff. locks
2020-02-11 22:10 ` [PATCH v4 3/4] xfs: Fix bug when checking diff. locks Pavel Reichl
@ 2020-02-12 0:38 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2020-02-12 0:38 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
On Tue, Feb 11, 2020 at 11:10:17PM +0100, Pavel Reichl wrote:
> In its current form, xfs_isilocked() is only able to test one lock type at a
> time - ilock, iolock, or mmap lock, but combinations are not properly handled.
> The intent here is to check that both XFS_IOLOCK_EXCL and XFS_ILOCK_EXCL are
> held, so test them each separately.
>
> The commit ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") ORed the flags
> together which was an error, so this patch reverts that part of the change and
> check the locks independently.
Commit message should wrap at 68-72 columns.
> Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
> Changelog from V3:
> Commit message extened.
Same comment as the previous patch about the subject - "fix" and
abbreviations.
xfs: xfs_isilocked() can only check a single lock type
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 4/4] xfs: Replace mrlock_t by rw_semaphore
2020-02-11 22:10 [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
2020-02-11 22:10 ` [PATCH v4 2/4] xfs: Fix WS in xfs_isilocked() calls Pavel Reichl
2020-02-11 22:10 ` [PATCH v4 3/4] xfs: Fix bug when checking diff. locks Pavel Reichl
@ 2020-02-11 22:10 ` Pavel Reichl
2020-02-12 0:49 ` Dave Chinner
2020-02-11 22:27 ` [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Eric Sandeen
3 siblings, 1 reply; 8+ messages in thread
From: Pavel Reichl @ 2020-02-11 22:10 UTC (permalink / raw)
To: linux-xfs
Remove mrlock_t as it does not provide any extra value over rw_semaphores.
Make i_lock and i_mmaplock native rw_semaphores and replace mr*() functions
with native rwsem calls.
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
fs/xfs/mrlock.h | 78 ----------------------------------------------
fs/xfs/xfs_inode.c | 37 +++++++++++-----------
fs/xfs/xfs_inode.h | 6 ++--
fs/xfs/xfs_iops.c | 4 +--
fs/xfs/xfs_linux.h | 1 -
fs/xfs/xfs_super.c | 6 ++--
6 files changed, 27 insertions(+), 105 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 be1f06fe1016..35a486cfa43c 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -191,14 +191,15 @@ xfs_ilock(
}
if (lock_flags & XFS_MMAPLOCK_EXCL)
- mrupdate_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
+ down_write_nested(&ip->i_mmaplock,
+ XFS_MMAPLOCK_DEP(lock_flags));
else if (lock_flags & XFS_MMAPLOCK_SHARED)
- mraccess_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
+ down_read_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
if (lock_flags & XFS_ILOCK_EXCL)
- mrupdate_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+ down_write_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
else if (lock_flags & XFS_ILOCK_SHARED)
- mraccess_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+ down_read_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
}
/*
@@ -242,27 +243,27 @@ xfs_ilock_nowait(
}
if (lock_flags & XFS_MMAPLOCK_EXCL) {
- if (!mrtryupdate(&ip->i_mmaplock))
+ if (!down_write_trylock(&ip->i_mmaplock))
goto out_undo_iolock;
} else if (lock_flags & XFS_MMAPLOCK_SHARED) {
- if (!mrtryaccess(&ip->i_mmaplock))
+ if (!down_read_trylock(&ip->i_mmaplock))
goto out_undo_iolock;
}
if (lock_flags & XFS_ILOCK_EXCL) {
- if (!mrtryupdate(&ip->i_lock))
+ if (!down_write_trylock(&ip->i_lock))
goto out_undo_mmaplock;
} else if (lock_flags & XFS_ILOCK_SHARED) {
- if (!mrtryaccess(&ip->i_lock))
+ if (!down_read_trylock(&ip->i_lock))
goto out_undo_mmaplock;
}
return 1;
out_undo_mmaplock:
if (lock_flags & XFS_MMAPLOCK_EXCL)
- mrunlock_excl(&ip->i_mmaplock);
+ up_write(&ip->i_mmaplock);
else if (lock_flags & XFS_MMAPLOCK_SHARED)
- mrunlock_shared(&ip->i_mmaplock);
+ up_read(&ip->i_mmaplock);
out_undo_iolock:
if (lock_flags & XFS_IOLOCK_EXCL)
up_write(&VFS_I(ip)->i_rwsem);
@@ -309,14 +310,14 @@ xfs_iunlock(
up_read(&VFS_I(ip)->i_rwsem);
if (lock_flags & XFS_MMAPLOCK_EXCL)
- mrunlock_excl(&ip->i_mmaplock);
+ up_write(&ip->i_mmaplock);
else if (lock_flags & XFS_MMAPLOCK_SHARED)
- mrunlock_shared(&ip->i_mmaplock);
+ up_read(&ip->i_mmaplock);
if (lock_flags & XFS_ILOCK_EXCL)
- mrunlock_excl(&ip->i_lock);
+ up_write(&ip->i_lock);
else if (lock_flags & XFS_ILOCK_SHARED)
- mrunlock_shared(&ip->i_lock);
+ up_read(&ip->i_lock);
trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
}
@@ -335,9 +336,9 @@ xfs_ilock_demote(
~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
if (lock_flags & XFS_ILOCK_EXCL)
- mrdemote(&ip->i_lock);
+ downgrade_write(&ip->i_lock);
if (lock_flags & XFS_MMAPLOCK_EXCL)
- mrdemote(&ip->i_mmaplock);
+ downgrade_write(&ip->i_mmaplock);
if (lock_flags & XFS_IOLOCK_EXCL)
downgrade_write(&VFS_I(ip)->i_rwsem);
@@ -375,14 +376,14 @@ xfs_isilocked(
{
if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)));
- return __xfs_rwsem_islocked(&ip->i_lock.mr_lock,
+ return __xfs_rwsem_islocked(&ip->i_lock,
(lock_flags & XFS_ILOCK_SHARED),
(lock_flags & XFS_ILOCK_EXCL));
}
if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
ASSERT(!(lock_flags & ~(XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)));
- return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock,
+ return __xfs_rwsem_islocked(&ip->i_mmaplock,
(lock_flags & XFS_MMAPLOCK_SHARED),
(lock_flags & XFS_MMAPLOCK_EXCL));
}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 3d7ce355407d..8b30f82b9dc0 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -9,6 +9,8 @@
#include "xfs_inode_buf.h"
#include "xfs_inode_fork.h"
+#include <linux/rwsem.h>
+
/*
* Kernel only inode definitions
*/
@@ -39,8 +41,8 @@ typedef struct xfs_inode {
/* Transaction and locking information. */
struct xfs_inode_log_item *i_itemp; /* logging information */
- mrlock_t i_lock; /* inode lock */
- mrlock_t i_mmaplock; /* inode mmap IO lock */
+ struct rw_semaphore i_lock; /* inode lock */
+ struct rw_semaphore i_mmaplock; /* inode mmap IO lock */
atomic_t i_pincount; /* inode pin count */
/*
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..7c3df574cf4c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1319,9 +1319,9 @@ xfs_setup_inode(
*/
lockdep_set_class(&inode->i_rwsem,
&inode->i_sb->s_type->i_mutex_dir_key);
- lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
+ lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class);
} else {
- lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
+ lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class);
}
/*
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 8738bb03f253..921a3eb093ed 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t;
#include "xfs_types.h"
#include "kmem.h"
-#include "mrlock.h"
#include <linux/semaphore.h>
#include <linux/mm.h>
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 760901783944..1289ce1f4e9e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -661,10 +661,8 @@ xfs_fs_inode_init_once(
atomic_set(&ip->i_pincount, 0);
spin_lock_init(&ip->i_flags_lock);
- mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
- "xfsino", ip->i_ino);
- mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
- "xfsino", ip->i_ino);
+ init_rwsem(&ip->i_mmaplock);
+ init_rwsem(&ip->i_lock);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 4/4] xfs: Replace mrlock_t by rw_semaphore
2020-02-11 22:10 ` [PATCH v4 4/4] xfs: Replace mrlock_t by rw_semaphore Pavel Reichl
@ 2020-02-12 0:49 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2020-02-12 0:49 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
On Tue, Feb 11, 2020 at 11:10:18PM +0100, Pavel Reichl wrote:
> Remove mrlock_t as it does not provide any extra value over rw_semaphores.
> Make i_lock and i_mmaplock native rw_semaphores and replace mr*() functions
> with native rwsem calls.
wrapping at 68-72 columns.
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
Subject "xfs: replace mrlock_t with rw_semaphores" or "xfs: remove
mrlock_t wrappers"
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 3d7ce355407d..8b30f82b9dc0 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -9,6 +9,8 @@
> #include "xfs_inode_buf.h"
> #include "xfs_inode_fork.h"
>
> +#include <linux/rwsem.h>
> +
Linux specific includes belong in fs/xfs/xfs_linux.h, not random XFS
header files.
Hmmm....
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 8738bb03f253..921a3eb093ed 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t;
> #include "xfs_types.h"
>
> #include "kmem.h"
> -#include "mrlock.h"
.... that's where rwsem.h currently gets included (via mrlock.h)
into the XFS codebase.
IOWs, the "#include <linux/rwsem.h>" should replace this include,
not get moved to xfs_inode.h.
Otherwise the patch looks fine.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/4] xfs: Refactor xfs_isilocked()
2020-02-11 22:10 [PATCH v4 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
` (2 preceding siblings ...)
2020-02-11 22:10 ` [PATCH v4 4/4] xfs: Replace mrlock_t by rw_semaphore Pavel Reichl
@ 2020-02-11 22:27 ` Eric Sandeen
3 siblings, 0 replies; 8+ messages in thread
From: Eric Sandeen @ 2020-02-11 22:27 UTC (permalink / raw)
To: Pavel Reichl, linux-xfs
On 2/11/20 4:10 PM, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> ---
> Changelog from V3:
> Added ASSERTS() to isilocked() to make sure that only flags for a single
> type of lock are passed
So while I like the ASSERTs going forward, the problem here is that a bisect
will now be broken between this and patch three. Sorry to do this to you,
but I think you should probably add the asserts in patch 3 so that you fix
the wrong calls and add the protective asserts at the same time.
Also, since your next patch fixes whitespace, I guess this:
+ ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)));
should observe the same rules around " | "
-Eric
>
> fs/xfs/xfs_inode.c | 51 ++++++++++++++++++++++++++++++++++------------
> fs/xfs/xfs_inode.h | 2 +-
> 2 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..cfefa7543b37 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -345,32 +345,57 @@ xfs_ilock_demote(
> }
>
> #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +static inline bool
> +__xfs_rwsem_islocked(
> + struct rw_semaphore *rwsem,
> + bool shared,
> + bool excl)
> +{
> + bool locked = false;
> +
> + if (!rwsem_is_locked(rwsem))
> + return false;
> +
> + if (!debug_locks)
> + return true;
> +
> + if (shared)
> + locked = lockdep_is_held_type(rwsem, 0);
> +
> + if (excl)
> + locked |= lockdep_is_held_type(rwsem, 1);
> +
> + return locked;
> +}
> +
> +bool
> xfs_isilocked(
> - xfs_inode_t *ip,
> + 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);
> + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)));
> + return __xfs_rwsem_islocked(&ip->i_lock.mr_lock,
> + (lock_flags & XFS_ILOCK_SHARED),
> + (lock_flags & XFS_ILOCK_EXCL));
> }
>
> if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> - if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> - return !!ip->i_mmaplock.mr_writer;
> - return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> + ASSERT(!(lock_flags & ~(XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)));
> + return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock,
> + (lock_flags & XFS_MMAPLOCK_SHARED),
> + (lock_flags & XFS_MMAPLOCK_EXCL));
> }
>
> if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
> - if (!(lock_flags & XFS_IOLOCK_SHARED))
> - return !debug_locks ||
> - lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0);
> - return rwsem_is_locked(&VFS_I(ip)->i_rwsem);
> + ASSERT(!(lock_flags & ~(XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)));
> + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
> + (lock_flags & XFS_IOLOCK_SHARED),
> + (lock_flags & XFS_IOLOCK_EXCL));
> }
>
> ASSERT(0);
> - return 0;
> + return false;
> }
> #endif
>
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 492e53992fa9..3d7ce355407d 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -416,7 +416,7 @@ void xfs_ilock(xfs_inode_t *, uint);
> int xfs_ilock_nowait(xfs_inode_t *, uint);
> void xfs_iunlock(xfs_inode_t *, uint);
> void xfs_ilock_demote(xfs_inode_t *, uint);
> -int xfs_isilocked(xfs_inode_t *, uint);
> +bool xfs_isilocked(xfs_inode_t *, uint);
> uint xfs_ilock_data_map_shared(struct xfs_inode *);
> uint xfs_ilock_attr_map_shared(struct xfs_inode *);
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread