* [PATCH v8 0/4] xfs: Remove wrappers for some semaphores
@ 2020-10-05 21:38 Pavel Reichl
2020-10-05 21:38 ` [PATCH v8 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Pavel Reichl @ 2020-10-05 21:38 UTC (permalink / raw)
To: linux-xfs
Remove some wrappers that we have in XFS around the read-write semaphore
locks.
The goal of this cleanup is to remove mrlock_t structure and its mr*()
wrapper functions and replace it with native rw_semaphore type and its
native calls.
Changes in version 8:
Patchset was rebased so it applies cleanly.
The patch 'xfs: replace mrlock_t with rw_semaphores' contains change in
xfs_btree.c which transfers ownership of lock so lockdep won't assert
(This was reported by Darrick and proposed change fixes this issue).
Pavel Reichl (4):
xfs: Refactor xfs_isilocked()
xfs: clean up whitespace in xfs_isilocked() calls
xfs: xfs_isilocked() can only check a single lock type
xfs: replace mrlock_t with rw_semaphores
fs/xfs/libxfs/xfs_bmap.c | 8 +--
fs/xfs/libxfs/xfs_btree.c | 10 ++++
fs/xfs/mrlock.h | 78 -----------------------------
fs/xfs/xfs_file.c | 3 +-
fs/xfs/xfs_inode.c | 102 +++++++++++++++++++++++++-------------
fs/xfs/xfs_inode.h | 25 ++++++----
fs/xfs/xfs_iops.c | 4 +-
fs/xfs/xfs_linux.h | 2 +-
fs/xfs/xfs_qm.c | 2 +-
fs/xfs/xfs_super.c | 6 +--
10 files changed, 107 insertions(+), 133 deletions(-)
delete mode 100644 fs/xfs/mrlock.h
--
2.26.2
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v8 1/4] xfs: Refactor xfs_isilocked() 2020-10-05 21:38 [PATCH v8 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl @ 2020-10-05 21:38 ` Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Pavel Reichl @ 2020-10-05 21:38 UTC (permalink / raw) To: linux-xfs Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). __xfs_rwsem_islocked() is a helper function which encapsulates checking state of rw_semaphores hold by inode. Signed-off-by: Pavel Reichl <preichl@redhat.com> Suggested-by: Dave Chinner <dchinner@redhat.com> Suggested-by: Eric Sandeen <sandeen@redhat.com> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/xfs_inode.c | 62 ++++++++++++++++++++++++++++++++++------------ fs/xfs/xfs_inode.h | 21 ++++++++++------ 2 files changed, 60 insertions(+), 23 deletions(-) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index c06129cffba9..1f39bce96656 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -345,32 +345,62 @@ xfs_ilock_demote( } #if defined(DEBUG) || defined(XFS_WARN) -int +static inline bool +__xfs_rwsem_islocked( + struct rw_semaphore *rwsem, + int lock_flags) +{ + int arg; + + if (!debug_locks) + return rwsem_is_locked(rwsem); + + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { + /* + * The caller could be asking if we have (shared | excl) + * access to the lock. Ask lockdep if the rwsem is + * locked either for read or write access. + * + * The caller could also be asking if we have only + * shared access to the lock. Holding a rwsem + * write-locked implies read access as well, so the + * request to lockdep is the same for this case. + */ + arg = -1; + } else { + /* + * The caller is asking if we have only exclusive access + * to the lock. Ask lockdep if the rwsem is locked for + * write access. + */ + arg = 0; + } + + return lockdep_is_held_type(rwsem, arg); +} + +bool xfs_isilocked( - xfs_inode_t *ip, + struct xfs_inode *ip, uint lock_flags) { - if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { - if (!(lock_flags & XFS_ILOCK_SHARED)) - return !!ip->i_lock.mr_writer; - return rwsem_is_locked(&ip->i_lock.mr_lock); + if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { + return __xfs_rwsem_islocked(&ip->i_lock, + (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); } - if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) { - if (!(lock_flags & XFS_MMAPLOCK_SHARED)) - return !!ip->i_mmaplock.mr_writer; - return rwsem_is_locked(&ip->i_mmaplock.mr_lock); + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { + return __xfs_rwsem_islocked(&ip->i_mmaplock, + (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); } - if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) { - if (!(lock_flags & XFS_IOLOCK_SHARED)) - return !debug_locks || - lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0); - return rwsem_is_locked(&VFS_I(ip)->i_rwsem); + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, + (lock_flags >> XFS_IOLOCK_FLAG_SHIFT)); } ASSERT(0); - return 0; + return false; } #endif diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index e9a8bb184d1f..77d5655191ab 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) * 1<<16 - 1<<32-1 -- lockdep annotation (integers) */ -#define XFS_IOLOCK_EXCL (1<<0) -#define XFS_IOLOCK_SHARED (1<<1) -#define XFS_ILOCK_EXCL (1<<2) -#define XFS_ILOCK_SHARED (1<<3) -#define XFS_MMAPLOCK_EXCL (1<<4) -#define XFS_MMAPLOCK_SHARED (1<<5) + +#define XFS_IOLOCK_FLAG_SHIFT 0 +#define XFS_ILOCK_FLAG_SHIFT 2 +#define XFS_MMAPLOCK_FLAG_SHIFT 4 + +#define XFS_SHARED_LOCK_SHIFT 1 + +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); int xfs_ilock_nowait(xfs_inode_t *, uint); void xfs_iunlock(xfs_inode_t *, uint); void xfs_ilock_demote(xfs_inode_t *, uint); -int xfs_isilocked(xfs_inode_t *, uint); +bool xfs_isilocked(xfs_inode_t *, uint); uint xfs_ilock_data_map_shared(struct xfs_inode *); uint xfs_ilock_attr_map_shared(struct xfs_inode *); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 1/4] xfs: Refactor xfs_isilocked() 2020-10-05 21:38 ` [PATCH v8 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl @ 2020-10-06 4:16 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2020-10-06 4:16 UTC (permalink / raw) To: Pavel Reichl; +Cc: linux-xfs On Mon, Oct 05, 2020 at 11:38:49PM +0200, Pavel Reichl wrote: > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > __xfs_rwsem_islocked() is a helper function which encapsulates checking > state of rw_semaphores hold by inode. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > Suggested-by: Dave Chinner <dchinner@redhat.com> > Suggested-by: Eric Sandeen <sandeen@redhat.com> > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_inode.c | 62 ++++++++++++++++++++++++++++++++++------------ > fs/xfs/xfs_inode.h | 21 ++++++++++------ > 2 files changed, 60 insertions(+), 23 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c06129cffba9..1f39bce96656 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -345,32 +345,62 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + int lock_flags) > +{ > + int arg; This ought ^^^^ to be indented ^^ to there. Otherwise this patch looks ok to me from what I vaguely remember from the last revision. > + > + if (!debug_locks) > + return rwsem_is_locked(rwsem); > + > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + /* > + * The caller could be asking if we have (shared | excl) > + * access to the lock. Ask lockdep if the rwsem is > + * locked either for read or write access. > + * > + * The caller could also be asking if we have only > + * shared access to the lock. Holding a rwsem > + * write-locked implies read access as well, so the > + * request to lockdep is the same for this case. > + */ > + arg = -1; > + } else { > + /* > + * The caller is asking if we have only exclusive access > + * to the lock. Ask lockdep if the rwsem is locked for > + * write access. Oh, also I appreciate the comment that documents why we're picking these seemingly magic numbers. --D > + */ > + arg = 0; > + } > + > + return lockdep_is_held_type(rwsem, arg); > +} > + > +bool > xfs_isilocked( > - xfs_inode_t *ip, > + struct xfs_inode *ip, > uint lock_flags) > { > - if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { > - if (!(lock_flags & XFS_ILOCK_SHARED)) > - return !!ip->i_lock.mr_writer; > - return rwsem_is_locked(&ip->i_lock.mr_lock); > + if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { > + return __xfs_rwsem_islocked(&ip->i_lock, > + (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); > } > > - if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) { > - if (!(lock_flags & XFS_MMAPLOCK_SHARED)) > - return !!ip->i_mmaplock.mr_writer; > - return rwsem_is_locked(&ip->i_mmaplock.mr_lock); > + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { > + return __xfs_rwsem_islocked(&ip->i_mmaplock, > + (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); > } > > - if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) { > - if (!(lock_flags & XFS_IOLOCK_SHARED)) > - return !debug_locks || > - lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0); > - return rwsem_is_locked(&VFS_I(ip)->i_rwsem); > + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { > + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, > + (lock_flags >> XFS_IOLOCK_FLAG_SHIFT)); > } > > ASSERT(0); > - return 0; > + return false; > } > #endif > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index e9a8bb184d1f..77d5655191ab 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) > * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) > * 1<<16 - 1<<32-1 -- lockdep annotation (integers) > */ > -#define XFS_IOLOCK_EXCL (1<<0) > -#define XFS_IOLOCK_SHARED (1<<1) > -#define XFS_ILOCK_EXCL (1<<2) > -#define XFS_ILOCK_SHARED (1<<3) > -#define XFS_MMAPLOCK_EXCL (1<<4) > -#define XFS_MMAPLOCK_SHARED (1<<5) > + > +#define XFS_IOLOCK_FLAG_SHIFT 0 > +#define XFS_ILOCK_FLAG_SHIFT 2 > +#define XFS_MMAPLOCK_FLAG_SHIFT 4 > + > +#define XFS_SHARED_LOCK_SHIFT 1 > + > +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) > +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) > +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) > +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ > | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ > @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); > int xfs_ilock_nowait(xfs_inode_t *, uint); > void xfs_iunlock(xfs_inode_t *, uint); > void xfs_ilock_demote(xfs_inode_t *, uint); > -int xfs_isilocked(xfs_inode_t *, uint); > +bool xfs_isilocked(xfs_inode_t *, uint); > uint xfs_ilock_data_map_shared(struct xfs_inode *); > uint xfs_ilock_attr_map_shared(struct xfs_inode *); > > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls 2020-10-05 21:38 [PATCH v8 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl 2020-10-05 21:38 ` [PATCH v8 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl @ 2020-10-05 21:38 ` Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl 2020-10-05 21:38 ` [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl 3 siblings, 1 reply; 13+ messages in thread From: Pavel Reichl @ 2020-10-05 21:38 UTC (permalink / raw) To: linux-xfs Make whitespace follow the same pattern in all xfs_isilocked() calls. 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 | 4 ++-- fs/xfs/xfs_qm.c | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 1b0a01b06a05..ced3b996cd8a 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3883,7 +3883,7 @@ xfs_bmapi_read( ASSERT(*nmap >= 1); ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE))); - ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)); if (WARN_ON_ONCE(!ifp)) return -EFSCORRUPTED; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index a29f78a663ca..c8b1d4e4199a 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -763,7 +763,8 @@ xfs_break_layouts( bool retry; int error; - ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)); + ASSERT(xfs_isilocked(XFS_I(inode), + XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)); do { retry = false; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 1f39bce96656..49d296877494 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2779,7 +2779,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_); @@ -3472,7 +3472,7 @@ xfs_iflush( struct xfs_mount *mp = ip->i_mount; int error; - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)); ASSERT(xfs_isiflocked(ip)); ASSERT(ip->i_df.if_format != XFS_DINODE_FMT_BTREE || ip->i_df.if_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK)); diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c index be67570badf8..57bfa5266c47 100644 --- a/fs/xfs/xfs_qm.c +++ b/fs/xfs/xfs_qm.c @@ -1803,7 +1803,7 @@ xfs_qm_vop_chown_reserve( int error; - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)); ASSERT(XFS_IS_QUOTA_RUNNING(mp)); delblks = ip->i_delayed_blks; -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls 2020-10-05 21:38 ` [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl @ 2020-10-06 4:16 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2020-10-06 4:16 UTC (permalink / raw) To: Pavel Reichl; +Cc: linux-xfs On Mon, Oct 05, 2020 at 11:38:50PM +0200, Pavel Reichl wrote: > Make whitespace follow the same pattern in all xfs_isilocked() calls. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/libxfs/xfs_bmap.c | 2 +- > fs/xfs/xfs_file.c | 3 ++- > fs/xfs/xfs_inode.c | 4 ++-- > fs/xfs/xfs_qm.c | 2 +- > 4 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 1b0a01b06a05..ced3b996cd8a 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3883,7 +3883,7 @@ xfs_bmapi_read( > > ASSERT(*nmap >= 1); > ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE))); > - ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)); > > if (WARN_ON_ONCE(!ifp)) > return -EFSCORRUPTED; > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index a29f78a663ca..c8b1d4e4199a 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -763,7 +763,8 @@ xfs_break_layouts( > bool retry; > int error; > > - ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)); > + ASSERT(xfs_isilocked(XFS_I(inode), > + XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)); > > do { > retry = false; > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 1f39bce96656..49d296877494 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2779,7 +2779,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_); > > @@ -3472,7 +3472,7 @@ xfs_iflush( > struct xfs_mount *mp = ip->i_mount; > int error; > > - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)); > ASSERT(xfs_isiflocked(ip)); > ASSERT(ip->i_df.if_format != XFS_DINODE_FMT_BTREE || > ip->i_df.if_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK)); > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c > index be67570badf8..57bfa5266c47 100644 > --- a/fs/xfs/xfs_qm.c > +++ b/fs/xfs/xfs_qm.c > @@ -1803,7 +1803,7 @@ xfs_qm_vop_chown_reserve( > int error; > > > - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)); > ASSERT(XFS_IS_QUOTA_RUNNING(mp)); > > delblks = ip->i_delayed_blks; > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type 2020-10-05 21:38 [PATCH v8 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl 2020-10-05 21:38 ` [PATCH v8 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl 2020-10-05 21:38 ` [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl @ 2020-10-05 21:38 ` Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl 3 siblings, 1 reply; 13+ messages in thread From: Pavel Reichl @ 2020-10-05 21:38 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") Suggested-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Pavel Reichl <preichl@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index ced3b996cd8a..ff5cc8a5d476 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -5787,7 +5787,8 @@ xfs_bmap_collapse_extents( if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL)); + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL)); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); if (!(ifp->if_flags & XFS_IFEXTENTS)) { error = xfs_iread_extents(tp, ip, whichfork); @@ -5904,7 +5905,8 @@ xfs_bmap_insert_extents( if (XFS_FORCED_SHUTDOWN(mp)) return -EIO; - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL)); + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL)); + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); if (!(ifp->if_flags & XFS_IFEXTENTS)) { error = xfs_iread_extents(tp, ip, whichfork); -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type 2020-10-05 21:38 ` [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl @ 2020-10-06 4:16 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2020-10-06 4:16 UTC (permalink / raw) To: Pavel Reichl; +Cc: linux-xfs On Mon, Oct 05, 2020 at 11:38:51PM +0200, Pavel Reichl wrote: > In its current form, xfs_isilocked() is only able to test one lock type > at a time - ilock, iolock, or mmap lock, but combinations are not > properly handled. The intent here is to check that both XFS_IOLOCK_EXCL > and XFS_ILOCK_EXCL are held, so test them each separately. > > The commit ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") ORed the > flags together which was an error, so this patch reverts that part of > the change and check the locks independently. > > Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") > > Suggested-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Pavel Reichl <preichl@redhat.com> Looks good, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/libxfs/xfs_bmap.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index ced3b996cd8a..ff5cc8a5d476 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -5787,7 +5787,8 @@ xfs_bmap_collapse_extents( > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL)); > + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL)); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > > if (!(ifp->if_flags & XFS_IFEXTENTS)) { > error = xfs_iread_extents(tp, ip, whichfork); > @@ -5904,7 +5905,8 @@ xfs_bmap_insert_extents( > if (XFS_FORCED_SHUTDOWN(mp)) > return -EIO; > > - ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL)); > + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL)); > + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL)); > > if (!(ifp->if_flags & XFS_IFEXTENTS)) { > error = xfs_iread_extents(tp, ip, whichfork); > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-05 21:38 [PATCH v8 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl ` (2 preceding siblings ...) 2020-10-05 21:38 ` [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl @ 2020-10-05 21:38 ` Pavel Reichl 2020-10-06 4:14 ` Darrick J. Wong 3 siblings, 1 reply; 13+ messages in thread From: Pavel Reichl @ 2020-10-05 21:38 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. Release the lock in xfs_btree_split() just before the work-queue executing xfs_btree_split_worker() is scheduled and make xfs_btree_split_worker() to acquire the lock as a first thing and release it just before returning from the function. This it done so the ownership of the lock is transfered between kernel threads and thus lockdep won't complain about lock being held by a different kernel thread. Signed-off-by: Pavel Reichl <preichl@redhat.com> --- fs/xfs/libxfs/xfs_btree.c | 10 +++++ fs/xfs/mrlock.h | 78 --------------------------------------- fs/xfs/xfs_inode.c | 36 ++++++++++-------- fs/xfs/xfs_inode.h | 4 +- fs/xfs/xfs_iops.c | 4 +- fs/xfs/xfs_linux.h | 2 +- fs/xfs/xfs_super.c | 6 +-- 7 files changed, 37 insertions(+), 103 deletions(-) delete mode 100644 fs/xfs/mrlock.h diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c index 2d25bab68764..d798d288eed1 100644 --- a/fs/xfs/libxfs/xfs_btree.c +++ b/fs/xfs/libxfs/xfs_btree.c @@ -2816,6 +2816,7 @@ xfs_btree_split_worker( unsigned long pflags; unsigned long new_pflags = PF_MEMALLOC_NOFS; + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); /* * we are in a transaction context here, but may also be doing work * in kswapd context, and hence we may need to inherit that state @@ -2832,6 +2833,7 @@ xfs_btree_split_worker( complete(args->done); current_restore_flags_nested(&pflags, new_pflags); + rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); } /* @@ -2863,8 +2865,16 @@ xfs_btree_split( args.done = &done; args.kswapd = current_is_kswapd(); INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker); + + /* Release the lock so it can be acquired in the kernel thread which + * executes the xfs_btree_split_worker. + */ + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); queue_work(xfs_alloc_wq, &args.work); + wait_for_completion(&done); + /* Re-aqcquire the lock previously released for the work queue. */ + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); destroy_work_on_stack(&args.work); return args.result; } diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h deleted file mode 100644 index 79155eec341b..000000000000 --- a/fs/xfs/mrlock.h +++ /dev/null @@ -1,78 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * Copyright (c) 2000-2006 Silicon Graphics, Inc. - * All Rights Reserved. - */ -#ifndef __XFS_SUPPORT_MRLOCK_H__ -#define __XFS_SUPPORT_MRLOCK_H__ - -#include <linux/rwsem.h> - -typedef struct { - struct rw_semaphore mr_lock; -#if defined(DEBUG) || defined(XFS_WARN) - int mr_writer; -#endif -} mrlock_t; - -#if defined(DEBUG) || defined(XFS_WARN) -#define mrinit(mrp, name) \ - do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0) -#else -#define mrinit(mrp, name) \ - do { init_rwsem(&(mrp)->mr_lock); } while (0) -#endif - -#define mrlock_init(mrp, t,n,s) mrinit(mrp, n) -#define mrfree(mrp) do { } while (0) - -static inline void mraccess_nested(mrlock_t *mrp, int subclass) -{ - down_read_nested(&mrp->mr_lock, subclass); -} - -static inline void mrupdate_nested(mrlock_t *mrp, int subclass) -{ - down_write_nested(&mrp->mr_lock, subclass); -#if defined(DEBUG) || defined(XFS_WARN) - mrp->mr_writer = 1; -#endif -} - -static inline int mrtryaccess(mrlock_t *mrp) -{ - return down_read_trylock(&mrp->mr_lock); -} - -static inline int mrtryupdate(mrlock_t *mrp) -{ - if (!down_write_trylock(&mrp->mr_lock)) - return 0; -#if defined(DEBUG) || defined(XFS_WARN) - mrp->mr_writer = 1; -#endif - return 1; -} - -static inline void mrunlock_excl(mrlock_t *mrp) -{ -#if defined(DEBUG) || defined(XFS_WARN) - mrp->mr_writer = 0; -#endif - up_write(&mrp->mr_lock); -} - -static inline void mrunlock_shared(mrlock_t *mrp) -{ - up_read(&mrp->mr_lock); -} - -static inline void mrdemote(mrlock_t *mrp) -{ -#if defined(DEBUG) || defined(XFS_WARN) - mrp->mr_writer = 0; -#endif - downgrade_write(&mrp->mr_lock); -} - -#endif /* __XFS_SUPPORT_MRLOCK_H__ */ diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 49d296877494..dbc1b80629c5 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); @@ -385,11 +386,14 @@ xfs_isilocked( uint lock_flags) { if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))); return __xfs_rwsem_islocked(&ip->i_lock, (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); } if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { + ASSERT(!(lock_flags & + ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))); return __xfs_rwsem_islocked(&ip->i_mmaplock, (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); } diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 77d5655191ab..02c98ecfe4c5 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -39,8 +39,8 @@ typedef struct xfs_inode { /* Transaction and locking information. */ struct xfs_inode_log_item *i_itemp; /* logging information */ - mrlock_t i_lock; /* inode lock */ - mrlock_t i_mmaplock; /* inode mmap IO lock */ + struct rw_semaphore i_lock; /* inode lock */ + struct rw_semaphore i_mmaplock; /* inode mmap IO lock */ atomic_t i_pincount; /* inode pin count */ /* diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 80a13c8561d8..66cca3e599c7 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -1336,9 +1336,9 @@ xfs_setup_inode( */ lockdep_set_class(&inode->i_rwsem, &inode->i_sb->s_type->i_mutex_dir_key); - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class); + lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class); } else { - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class); + lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class); } /* diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index ab737fed7b12..ba37217f86d2 100644 --- a/fs/xfs/xfs_linux.h +++ b/fs/xfs/xfs_linux.h @@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t; #include "xfs_types.h" #include "kmem.h" -#include "mrlock.h" #include <linux/semaphore.h> #include <linux/mm.h> @@ -61,6 +60,7 @@ typedef __u32 xfs_nlink_t; #include <linux/ratelimit.h> #include <linux/rhashtable.h> #include <linux/xattr.h> +#include <linux/rwsem.h> #include <asm/page.h> #include <asm/div64.h> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 71ac6c1cdc36..00be9cfa29fa 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -708,10 +708,8 @@ xfs_fs_inode_init_once( atomic_set(&ip->i_pincount, 0); spin_lock_init(&ip->i_flags_lock); - mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, - "xfsino", ip->i_ino); - mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, - "xfsino", ip->i_ino); + init_rwsem(&ip->i_mmaplock); + init_rwsem(&ip->i_lock); } /* -- 2.26.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-05 21:38 ` [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl @ 2020-10-06 4:14 ` Darrick J. Wong 2020-10-06 10:50 ` Pavel Reichl 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-10-06 4:14 UTC (permalink / raw) To: Pavel Reichl; +Cc: linux-xfs On Mon, Oct 05, 2020 at 11:38:52PM +0200, Pavel Reichl wrote: > Remove mrlock_t as it does not provide any extra value over > rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and > replace mr*() functions with native rwsem calls. > > Release the lock in xfs_btree_split() just before the work-queue > executing xfs_btree_split_worker() is scheduled and make > xfs_btree_split_worker() to acquire the lock as a first thing and > release it just before returning from the function. This it done so the > ownership of the lock is transfered between kernel threads and thus > lockdep won't complain about lock being held by a different kernel > thread. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > fs/xfs/libxfs/xfs_btree.c | 10 +++++ > fs/xfs/mrlock.h | 78 --------------------------------------- > fs/xfs/xfs_inode.c | 36 ++++++++++-------- > fs/xfs/xfs_inode.h | 4 +- > fs/xfs/xfs_iops.c | 4 +- > fs/xfs/xfs_linux.h | 2 +- > fs/xfs/xfs_super.c | 6 +-- > 7 files changed, 37 insertions(+), 103 deletions(-) > delete mode 100644 fs/xfs/mrlock.h > > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c > index 2d25bab68764..d798d288eed1 100644 > --- a/fs/xfs/libxfs/xfs_btree.c > +++ b/fs/xfs/libxfs/xfs_btree.c > @@ -2816,6 +2816,7 @@ xfs_btree_split_worker( > unsigned long pflags; > unsigned long new_pflags = PF_MEMALLOC_NOFS; > > + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); > /* > * we are in a transaction context here, but may also be doing work > * in kswapd context, and hence we may need to inherit that state > @@ -2832,6 +2833,7 @@ xfs_btree_split_worker( > complete(args->done); > > current_restore_flags_nested(&pflags, new_pflags); > + rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); > } > > /* > @@ -2863,8 +2865,16 @@ xfs_btree_split( > args.done = &done; > args.kswapd = current_is_kswapd(); > INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker); > + > + /* Release the lock so it can be acquired in the kernel thread which Strange comment style. Also, we're not really releasing the lock itself here, right? We're merely updating lockdep's bookkeepping so that the worker can make itself look like the lock owner (to lockdep, anyway). Does this exist as a helper anywhere in the kernel? I don't really like XFS poking into the rw_semaphore innards, though I concede that this lock transferring dance is probably pretty rare. --D > + * executes the xfs_btree_split_worker. > + */ > + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); > queue_work(xfs_alloc_wq, &args.work); > + > wait_for_completion(&done); > + /* Re-aqcquire the lock previously released for the work queue. */ > + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); > destroy_work_on_stack(&args.work); > return args.result; > } > diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h > deleted file mode 100644 > index 79155eec341b..000000000000 > --- a/fs/xfs/mrlock.h > +++ /dev/null > @@ -1,78 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * Copyright (c) 2000-2006 Silicon Graphics, Inc. > - * All Rights Reserved. > - */ > -#ifndef __XFS_SUPPORT_MRLOCK_H__ > -#define __XFS_SUPPORT_MRLOCK_H__ > - > -#include <linux/rwsem.h> > - > -typedef struct { > - struct rw_semaphore mr_lock; > -#if defined(DEBUG) || defined(XFS_WARN) > - int mr_writer; > -#endif > -} mrlock_t; > - > -#if defined(DEBUG) || defined(XFS_WARN) > -#define mrinit(mrp, name) \ > - do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0) > -#else > -#define mrinit(mrp, name) \ > - do { init_rwsem(&(mrp)->mr_lock); } while (0) > -#endif > - > -#define mrlock_init(mrp, t,n,s) mrinit(mrp, n) > -#define mrfree(mrp) do { } while (0) > - > -static inline void mraccess_nested(mrlock_t *mrp, int subclass) > -{ > - down_read_nested(&mrp->mr_lock, subclass); > -} > - > -static inline void mrupdate_nested(mrlock_t *mrp, int subclass) > -{ > - down_write_nested(&mrp->mr_lock, subclass); > -#if defined(DEBUG) || defined(XFS_WARN) > - mrp->mr_writer = 1; > -#endif > -} > - > -static inline int mrtryaccess(mrlock_t *mrp) > -{ > - return down_read_trylock(&mrp->mr_lock); > -} > - > -static inline int mrtryupdate(mrlock_t *mrp) > -{ > - if (!down_write_trylock(&mrp->mr_lock)) > - return 0; > -#if defined(DEBUG) || defined(XFS_WARN) > - mrp->mr_writer = 1; > -#endif > - return 1; > -} > - > -static inline void mrunlock_excl(mrlock_t *mrp) > -{ > -#if defined(DEBUG) || defined(XFS_WARN) > - mrp->mr_writer = 0; > -#endif > - up_write(&mrp->mr_lock); > -} > - > -static inline void mrunlock_shared(mrlock_t *mrp) > -{ > - up_read(&mrp->mr_lock); > -} > - > -static inline void mrdemote(mrlock_t *mrp) > -{ > -#if defined(DEBUG) || defined(XFS_WARN) > - mrp->mr_writer = 0; > -#endif > - downgrade_write(&mrp->mr_lock); > -} > - > -#endif /* __XFS_SUPPORT_MRLOCK_H__ */ > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 49d296877494..dbc1b80629c5 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); > > @@ -385,11 +386,14 @@ xfs_isilocked( > uint lock_flags) > { > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { > + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))); > return __xfs_rwsem_islocked(&ip->i_lock, > (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); > } > > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { > + ASSERT(!(lock_flags & > + ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))); > return __xfs_rwsem_islocked(&ip->i_mmaplock, > (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); > } > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 77d5655191ab..02c98ecfe4c5 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -39,8 +39,8 @@ typedef struct xfs_inode { > > /* Transaction and locking information. */ > struct xfs_inode_log_item *i_itemp; /* logging information */ > - mrlock_t i_lock; /* inode lock */ > - mrlock_t i_mmaplock; /* inode mmap IO lock */ > + struct rw_semaphore i_lock; /* inode lock */ > + struct rw_semaphore i_mmaplock; /* inode mmap IO lock */ > atomic_t i_pincount; /* inode pin count */ > > /* > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index 80a13c8561d8..66cca3e599c7 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -1336,9 +1336,9 @@ xfs_setup_inode( > */ > lockdep_set_class(&inode->i_rwsem, > &inode->i_sb->s_type->i_mutex_dir_key); > - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class); > + lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class); > } else { > - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class); > + lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class); > } > > /* > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h > index ab737fed7b12..ba37217f86d2 100644 > --- a/fs/xfs/xfs_linux.h > +++ b/fs/xfs/xfs_linux.h > @@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t; > #include "xfs_types.h" > > #include "kmem.h" > -#include "mrlock.h" > > #include <linux/semaphore.h> > #include <linux/mm.h> > @@ -61,6 +60,7 @@ typedef __u32 xfs_nlink_t; > #include <linux/ratelimit.h> > #include <linux/rhashtable.h> > #include <linux/xattr.h> > +#include <linux/rwsem.h> > > #include <asm/page.h> > #include <asm/div64.h> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 71ac6c1cdc36..00be9cfa29fa 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -708,10 +708,8 @@ xfs_fs_inode_init_once( > atomic_set(&ip->i_pincount, 0); > spin_lock_init(&ip->i_flags_lock); > > - mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, > - "xfsino", ip->i_ino); > - mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, > - "xfsino", ip->i_ino); > + init_rwsem(&ip->i_mmaplock); > + init_rwsem(&ip->i_lock); > } > > /* > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-06 4:14 ` Darrick J. Wong @ 2020-10-06 10:50 ` Pavel Reichl 2020-10-06 13:54 ` Eric Sandeen 2020-10-06 14:04 ` Eric Sandeen 0 siblings, 2 replies; 13+ messages in thread From: Pavel Reichl @ 2020-10-06 10:50 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On 10/6/20 6:14 AM, Darrick J. Wong wrote: > On Mon, Oct 05, 2020 at 11:38:52PM +0200, Pavel Reichl wrote: >> Remove mrlock_t as it does not provide any extra value over >> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and >> replace mr*() functions with native rwsem calls. >> >> Release the lock in xfs_btree_split() just before the work-queue >> executing xfs_btree_split_worker() is scheduled and make >> xfs_btree_split_worker() to acquire the lock as a first thing and >> release it just before returning from the function. This it done so the >> ownership of the lock is transfered between kernel threads and thus >> lockdep won't complain about lock being held by a different kernel >> thread. >> >> Signed-off-by: Pavel Reichl <preichl@redhat.com> >> --- >> fs/xfs/libxfs/xfs_btree.c | 10 +++++ >> fs/xfs/mrlock.h | 78 --------------------------------------- >> fs/xfs/xfs_inode.c | 36 ++++++++++-------- >> fs/xfs/xfs_inode.h | 4 +- >> fs/xfs/xfs_iops.c | 4 +- >> fs/xfs/xfs_linux.h | 2 +- >> fs/xfs/xfs_super.c | 6 +-- >> 7 files changed, 37 insertions(+), 103 deletions(-) >> delete mode 100644 fs/xfs/mrlock.h >> >> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c >> index 2d25bab68764..d798d288eed1 100644 >> --- a/fs/xfs/libxfs/xfs_btree.c >> +++ b/fs/xfs/libxfs/xfs_btree.c >> @@ -2816,6 +2816,7 @@ xfs_btree_split_worker( >> unsigned long pflags; >> unsigned long new_pflags = PF_MEMALLOC_NOFS; >> >> + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); >> /* >> * we are in a transaction context here, but may also be doing work >> * in kswapd context, and hence we may need to inherit that state >> @@ -2832,6 +2833,7 @@ xfs_btree_split_worker( >> complete(args->done); >> >> current_restore_flags_nested(&pflags, new_pflags); >> + rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); >> } >> >> /* >> @@ -2863,8 +2865,16 @@ xfs_btree_split( >> args.done = &done; >> args.kswapd = current_is_kswapd(); >> INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker); >> + >> + /* Release the lock so it can be acquired in the kernel thread which > > Strange comment style. OK, I'll try to think about something better, but ideas are welcome :-). > > Also, we're not really releasing the lock itself here, right? We're > merely updating lockdep's bookkeepping so that the worker can make > itself look like the lock owner (to lockdep, anyway). Hmm...I'm afraid I don't follow - yes we are doing this to satisfy lockdep's bookkeeping, however we actually do this by releasing the lock in one kernel thread and acquiring it in another. > > Does this exist as a helper anywhere in the kernel? I don't really like > XFS poking into the rw_semaphore innards, though I concede that this > lock transferring dance is probably pretty rare. I'll try to look for it. > > --D > >> + * executes the xfs_btree_split_worker. >> + */ >> + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); >> queue_work(xfs_alloc_wq, &args.work); >> + >> wait_for_completion(&done); >> + /* Re-aqcquire the lock previously released for the work queue. */ >> + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); >> destroy_work_on_stack(&args.work); >> return args.result; >> } >> diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h >> deleted file mode 100644 >> index 79155eec341b..000000000000 >> --- a/fs/xfs/mrlock.h >> +++ /dev/null >> @@ -1,78 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0 >> -/* >> - * Copyright (c) 2000-2006 Silicon Graphics, Inc. >> - * All Rights Reserved. >> - */ >> -#ifndef __XFS_SUPPORT_MRLOCK_H__ >> -#define __XFS_SUPPORT_MRLOCK_H__ >> - >> -#include <linux/rwsem.h> >> - >> -typedef struct { >> - struct rw_semaphore mr_lock; >> -#if defined(DEBUG) || defined(XFS_WARN) >> - int mr_writer; >> -#endif >> -} mrlock_t; >> - >> -#if defined(DEBUG) || defined(XFS_WARN) >> -#define mrinit(mrp, name) \ >> - do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0) >> -#else >> -#define mrinit(mrp, name) \ >> - do { init_rwsem(&(mrp)->mr_lock); } while (0) >> -#endif >> - >> -#define mrlock_init(mrp, t,n,s) mrinit(mrp, n) >> -#define mrfree(mrp) do { } while (0) >> - >> -static inline void mraccess_nested(mrlock_t *mrp, int subclass) >> -{ >> - down_read_nested(&mrp->mr_lock, subclass); >> -} >> - >> -static inline void mrupdate_nested(mrlock_t *mrp, int subclass) >> -{ >> - down_write_nested(&mrp->mr_lock, subclass); >> -#if defined(DEBUG) || defined(XFS_WARN) >> - mrp->mr_writer = 1; >> -#endif >> -} >> - >> -static inline int mrtryaccess(mrlock_t *mrp) >> -{ >> - return down_read_trylock(&mrp->mr_lock); >> -} >> - >> -static inline int mrtryupdate(mrlock_t *mrp) >> -{ >> - if (!down_write_trylock(&mrp->mr_lock)) >> - return 0; >> -#if defined(DEBUG) || defined(XFS_WARN) >> - mrp->mr_writer = 1; >> -#endif >> - return 1; >> -} >> - >> -static inline void mrunlock_excl(mrlock_t *mrp) >> -{ >> -#if defined(DEBUG) || defined(XFS_WARN) >> - mrp->mr_writer = 0; >> -#endif >> - up_write(&mrp->mr_lock); >> -} >> - >> -static inline void mrunlock_shared(mrlock_t *mrp) >> -{ >> - up_read(&mrp->mr_lock); >> -} >> - >> -static inline void mrdemote(mrlock_t *mrp) >> -{ >> -#if defined(DEBUG) || defined(XFS_WARN) >> - mrp->mr_writer = 0; >> -#endif >> - downgrade_write(&mrp->mr_lock); >> -} >> - >> -#endif /* __XFS_SUPPORT_MRLOCK_H__ */ >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c >> index 49d296877494..dbc1b80629c5 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); >> >> @@ -385,11 +386,14 @@ xfs_isilocked( >> uint lock_flags) >> { >> if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { >> + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))); >> return __xfs_rwsem_islocked(&ip->i_lock, >> (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); >> } >> >> if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { >> + ASSERT(!(lock_flags & >> + ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))); >> return __xfs_rwsem_islocked(&ip->i_mmaplock, >> (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); >> } >> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h >> index 77d5655191ab..02c98ecfe4c5 100644 >> --- a/fs/xfs/xfs_inode.h >> +++ b/fs/xfs/xfs_inode.h >> @@ -39,8 +39,8 @@ typedef struct xfs_inode { >> >> /* Transaction and locking information. */ >> struct xfs_inode_log_item *i_itemp; /* logging information */ >> - mrlock_t i_lock; /* inode lock */ >> - mrlock_t i_mmaplock; /* inode mmap IO lock */ >> + struct rw_semaphore i_lock; /* inode lock */ >> + struct rw_semaphore i_mmaplock; /* inode mmap IO lock */ >> atomic_t i_pincount; /* inode pin count */ >> >> /* >> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c >> index 80a13c8561d8..66cca3e599c7 100644 >> --- a/fs/xfs/xfs_iops.c >> +++ b/fs/xfs/xfs_iops.c >> @@ -1336,9 +1336,9 @@ xfs_setup_inode( >> */ >> lockdep_set_class(&inode->i_rwsem, >> &inode->i_sb->s_type->i_mutex_dir_key); >> - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class); >> + lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class); >> } else { >> - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class); >> + lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class); >> } >> >> /* >> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h >> index ab737fed7b12..ba37217f86d2 100644 >> --- a/fs/xfs/xfs_linux.h >> +++ b/fs/xfs/xfs_linux.h >> @@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t; >> #include "xfs_types.h" >> >> #include "kmem.h" >> -#include "mrlock.h" >> >> #include <linux/semaphore.h> >> #include <linux/mm.h> >> @@ -61,6 +60,7 @@ typedef __u32 xfs_nlink_t; >> #include <linux/ratelimit.h> >> #include <linux/rhashtable.h> >> #include <linux/xattr.h> >> +#include <linux/rwsem.h> >> >> #include <asm/page.h> >> #include <asm/div64.h> >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c >> index 71ac6c1cdc36..00be9cfa29fa 100644 >> --- a/fs/xfs/xfs_super.c >> +++ b/fs/xfs/xfs_super.c >> @@ -708,10 +708,8 @@ xfs_fs_inode_init_once( >> atomic_set(&ip->i_pincount, 0); >> spin_lock_init(&ip->i_flags_lock); >> >> - mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, >> - "xfsino", ip->i_ino); >> - mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER, >> - "xfsino", ip->i_ino); >> + init_rwsem(&ip->i_mmaplock); >> + init_rwsem(&ip->i_lock); >> } >> >> /* >> -- >> 2.26.2 >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-06 10:50 ` Pavel Reichl @ 2020-10-06 13:54 ` Eric Sandeen 2020-10-06 14:04 ` Eric Sandeen 1 sibling, 0 replies; 13+ messages in thread From: Eric Sandeen @ 2020-10-06 13:54 UTC (permalink / raw) To: Pavel Reichl, Darrick J. Wong; +Cc: linux-xfs On 10/6/20 5:50 AM, Pavel Reichl wrote: > > > On 10/6/20 6:14 AM, Darrick J. Wong wrote: >> On Mon, Oct 05, 2020 at 11:38:52PM +0200, Pavel Reichl wrote: >>> Remove mrlock_t as it does not provide any extra value over >>> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and >>> replace mr*() functions with native rwsem calls. >>> >>> Release the lock in xfs_btree_split() just before the work-queue >>> executing xfs_btree_split_worker() is scheduled and make >>> xfs_btree_split_worker() to acquire the lock as a first thing and >>> release it just before returning from the function. This it done so the >>> ownership of the lock is transfered between kernel threads and thus >>> lockdep won't complain about lock being held by a different kernel >>> thread. >>> >>> Signed-off-by: Pavel Reichl <preichl@redhat.com> >>> --- >>> fs/xfs/libxfs/xfs_btree.c | 10 +++++ >>> fs/xfs/mrlock.h | 78 --------------------------------------- >>> fs/xfs/xfs_inode.c | 36 ++++++++++-------- >>> fs/xfs/xfs_inode.h | 4 +- >>> fs/xfs/xfs_iops.c | 4 +- >>> fs/xfs/xfs_linux.h | 2 +- >>> fs/xfs/xfs_super.c | 6 +-- >>> 7 files changed, 37 insertions(+), 103 deletions(-) >>> delete mode 100644 fs/xfs/mrlock.h >>> >>> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c >>> index 2d25bab68764..d798d288eed1 100644 >>> --- a/fs/xfs/libxfs/xfs_btree.c >>> +++ b/fs/xfs/libxfs/xfs_btree.c >>> @@ -2816,6 +2816,7 @@ xfs_btree_split_worker( >>> unsigned long pflags; >>> unsigned long new_pflags = PF_MEMALLOC_NOFS; >>> >>> + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); >>> /* >>> * we are in a transaction context here, but may also be doing work >>> * in kswapd context, and hence we may need to inherit that state >>> @@ -2832,6 +2833,7 @@ xfs_btree_split_worker( >>> complete(args->done); >>> >>> current_restore_flags_nested(&pflags, new_pflags); >>> + rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); >>> } >>> >>> /* >>> @@ -2863,8 +2865,16 @@ xfs_btree_split( >>> args.done = &done; >>> args.kswapd = current_is_kswapd(); >>> INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker); >>> + >>> + /* Release the lock so it can be acquired in the kernel thread which >> >> Strange comment style. > > OK, I'll try to think about something better, but ideas are welcome :-). /* * Typical multi-line XFS comments are like this, with no * comment text on the opening or closing lines */ /* Single-line comments are fine like this though */ -Eric ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-06 10:50 ` Pavel Reichl 2020-10-06 13:54 ` Eric Sandeen @ 2020-10-06 14:04 ` Eric Sandeen 2020-10-06 15:28 ` Darrick J. Wong 1 sibling, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2020-10-06 14:04 UTC (permalink / raw) To: Pavel Reichl, Darrick J. Wong; +Cc: linux-xfs On 10/6/20 5:50 AM, Pavel Reichl wrote: >> Also, we're not really releasing the lock itself here, right? We're >> merely updating lockdep's bookkeepping so that the worker can make >> itself look like the lock owner (to lockdep, anyway). > Hmm...I'm afraid I don't follow - yes we are doing this to satisfy lockdep's bookkeeping, > however we actually do this by releasing the lock in one kernel thread and acquiring it in another. it's the difference between actually releasing the lock itself, and telling lockdep that we're releasing the "ownership" of the lock for tracking purposes; I agree that "rwsem_release" is a bit confusingly named. > >> Does this exist as a helper anywhere in the kernel? I don't really like >> XFS poking into the rw_semaphore innards, though I concede that this >> lock transferring dance is probably pretty rare. > I'll try to look for it. > Other code I see just calls rwsem_release directly - ocfs2, jbd2, kernfs etc. I think a clearer comment might suffice, not sure what Darrick thinks, maybe something like this: + /* + * Let lockdep know that we won't own i_lock when we hand off + * to the worker thread + */ + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); queue_work(xfs_alloc_wq, &args.work); + wait_for_completion(&done); + /* We own the i_lock again */ + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); and similar comments in the worker: + /* Let lockdep know that we own the i_lock for now */ + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); ... etc -Eric ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores 2020-10-06 14:04 ` Eric Sandeen @ 2020-10-06 15:28 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2020-10-06 15:28 UTC (permalink / raw) To: Eric Sandeen; +Cc: Pavel Reichl, linux-xfs On Tue, Oct 06, 2020 at 09:04:18AM -0500, Eric Sandeen wrote: > > > On 10/6/20 5:50 AM, Pavel Reichl wrote: > >> Also, we're not really releasing the lock itself here, right? We're > >> merely updating lockdep's bookkeepping so that the worker can make > >> itself look like the lock owner (to lockdep, anyway). > > Hmm...I'm afraid I don't follow - yes we are doing this to satisfy lockdep's bookkeeping, > > however we actually do this by releasing the lock in one kernel thread and acquiring it in another. > > it's the difference between actually releasing the lock itself, and > telling lockdep that we're releasing the "ownership" of the lock for tracking > purposes; I agree that "rwsem_release" is a bit confusingly named. Yes. > > > >> Does this exist as a helper anywhere in the kernel? I don't really like > >> XFS poking into the rw_semaphore innards, though I concede that this > >> lock transferring dance is probably pretty rare. > > I'll try to look for it. > > > > Other code I see just calls rwsem_release directly - ocfs2, jbd2, kernfs etc. > > I think a clearer comment might suffice, not sure what Darrick thinks, maybe something > like this: > > + /* > + * Let lockdep know that we won't own i_lock when we hand off > + * to the worker thread > + */ /* * Update lockdep's ownership information to reflect that we * will be transferring the ilock from this thread to the * worker. */ > + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_); > queue_work(xfs_alloc_wq, &args.work); > + > wait_for_completion(&done); > + /* We own the i_lock again */ /* * Update lockdep's lock ownership information to point to * this thread as the lock owner now that the worker item is * done. */ Perhaps? --D > + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); > > and similar comments in the worker: > > + /* Let lockdep know that we own the i_lock for now */ > + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_); > ... > > etc > > -Eric ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-10-06 15:28 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-05 21:38 [PATCH v8 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl 2020-10-05 21:38 ` [PATCH v8 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl 2020-10-06 4:16 ` Darrick J. Wong 2020-10-05 21:38 ` [PATCH v8 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl 2020-10-06 4:14 ` Darrick J. Wong 2020-10-06 10:50 ` Pavel Reichl 2020-10-06 13:54 ` Eric Sandeen 2020-10-06 14:04 ` Eric Sandeen 2020-10-06 15:28 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.