| Message ID | 20201009195515.82889-2-preichl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: Remove wrappers for some semaphores | expand |
On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > __xfs_rwsem_islocked() is a helper function which encapsulates checking > state of rw_semaphores hold by inode. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > Suggested-by: Dave Chinner <dchinner@redhat.com> > Suggested-by: Eric Sandeen <sandeen@redhat.com> > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > fs/xfs/xfs_inode.h | 21 +++++++++++++------- > 2 files changed, 54 insertions(+), 15 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c06129cffba9..7c1ceb4df4ec 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -345,9 +345,43 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + int lock_flags) > +{ > + int arg; > + > + if (!debug_locks) > + return rwsem_is_locked(rwsem); > + > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + /* > + * The caller could be asking if we have (shared | excl) > + * access to the lock. Ask lockdep if the rwsem is > + * locked either for read or write access. > + * > + * The caller could also be asking if we have only > + * shared access to the lock. Holding a rwsem > + * write-locked implies read access as well, so the > + * request to lockdep is the same for this case. > + */ > + arg = -1; > + } else { > + /* > + * The caller is asking if we have only exclusive access > + * to the lock. Ask lockdep if the rwsem is locked for > + * write access. > + */ > + arg = 0; > + } Are these arg values documented somewhere? A quick look at the function below didn't show anything.. Also, I find the pattern of shifting in the caller slightly confusing, particularly with the 'lock_flags' name being passed down through the caller. Any reason we couldn't pass the shift value as a parameter and do the shift at the top of the function so the logic is clear and in one place? > + > + return lockdep_is_held_type(rwsem, arg); > +} > + > +bool > xfs_isilocked( > - xfs_inode_t *ip, > + struct xfs_inode *ip, > uint lock_flags) > { > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { ... > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index e9a8bb184d1f..77776af75c77 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) > * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) > * 1<<16 - 1<<32-1 -- lockdep annotation (integers) > */ > -#define XFS_IOLOCK_EXCL (1<<0) > -#define XFS_IOLOCK_SHARED (1<<1) > -#define XFS_ILOCK_EXCL (1<<2) > -#define XFS_ILOCK_SHARED (1<<3) > -#define XFS_MMAPLOCK_EXCL (1<<4) > -#define XFS_MMAPLOCK_SHARED (1<<5) > + > +#define XFS_IOLOCK_FLAG_SHIFT 0 > +#define XFS_ILOCK_FLAG_SHIFT 2 > +#define XFS_MMAPLOCK_FLAG_SHIFT 4 > + > +#define XFS_SHARED_LOCK_SHIFT 1 > + > +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) > +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) > +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) > +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > Any reason for the extra params around the shift values? Brian > #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ > | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ > @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); > int xfs_ilock_nowait(xfs_inode_t *, uint); > void xfs_iunlock(xfs_inode_t *, uint); > void xfs_ilock_demote(xfs_inode_t *, uint); > -int xfs_isilocked(xfs_inode_t *, uint); > +bool xfs_isilocked(struct xfs_inode *, uint); > uint xfs_ilock_data_map_shared(struct xfs_inode *); > uint xfs_ilock_attr_map_shared(struct xfs_inode *); > > -- > 2.26.2 >
On Mon, Oct 12, 2020 at 12:03:08PM -0400, Brian Foster wrote: > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: > > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > > __xfs_rwsem_islocked() is a helper function which encapsulates checking > > state of rw_semaphores hold by inode. > > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > Suggested-by: Dave Chinner <dchinner@redhat.com> > > Suggested-by: Eric Sandeen <sandeen@redhat.com> > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > > fs/xfs/xfs_inode.h | 21 +++++++++++++------- > > 2 files changed, 54 insertions(+), 15 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index c06129cffba9..7c1ceb4df4ec 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -345,9 +345,43 @@ xfs_ilock_demote( > > } > > > > #if defined(DEBUG) || defined(XFS_WARN) > > -int > > +static inline bool > > +__xfs_rwsem_islocked( > > + struct rw_semaphore *rwsem, > > + int lock_flags) > > +{ > > + int arg; > > + > > + if (!debug_locks) > > + return rwsem_is_locked(rwsem); > > + > > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > > + /* > > + * The caller could be asking if we have (shared | excl) > > + * access to the lock. Ask lockdep if the rwsem is > > + * locked either for read or write access. > > + * > > + * The caller could also be asking if we have only > > + * shared access to the lock. Holding a rwsem > > + * write-locked implies read access as well, so the > > + * request to lockdep is the same for this case. > > + */ > > + arg = -1; > > + } else { > > + /* > > + * The caller is asking if we have only exclusive access > > + * to the lock. Ask lockdep if the rwsem is locked for > > + * write access. > > + */ > > + arg = 0; > > + } > > Are these arg values documented somewhere? A quick look at the function > below didn't show anything.. Alas, no. :( If you trace lockdep_is_held_type -> lock_is_held_type -> __lock_is_held then you'll notice that "if (read == -1" bit, but none of those functions are documented. So I have no if that's /really/ permanent, other than to say that it exists because Dave and Christoph and I requested it years ago and commit f8319483f57f1 has been unchanged since 2016. --D > Also, I find the pattern of shifting in the caller slightly confusing, > particularly with the 'lock_flags' name being passed down through the > caller. Any reason we couldn't pass the shift value as a parameter and > do the shift at the top of the function so the logic is clear and in one > place? > > > + > > + return lockdep_is_held_type(rwsem, arg); > > +} > > + > > +bool > > xfs_isilocked( > > - xfs_inode_t *ip, > > + struct xfs_inode *ip, > > uint lock_flags) > > { > > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { > ... > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > > index e9a8bb184d1f..77776af75c77 100644 > > --- a/fs/xfs/xfs_inode.h > > +++ b/fs/xfs/xfs_inode.h > > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) > > * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) > > * 1<<16 - 1<<32-1 -- lockdep annotation (integers) > > */ > > -#define XFS_IOLOCK_EXCL (1<<0) > > -#define XFS_IOLOCK_SHARED (1<<1) > > -#define XFS_ILOCK_EXCL (1<<2) > > -#define XFS_ILOCK_SHARED (1<<3) > > -#define XFS_MMAPLOCK_EXCL (1<<4) > > -#define XFS_MMAPLOCK_SHARED (1<<5) > > + > > +#define XFS_IOLOCK_FLAG_SHIFT 0 > > +#define XFS_ILOCK_FLAG_SHIFT 2 > > +#define XFS_MMAPLOCK_FLAG_SHIFT 4 > > + > > +#define XFS_SHARED_LOCK_SHIFT 1 > > + > > +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) > > +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) > > +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) > > +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > > > Any reason for the extra params around the shift values? > > Brian > > > #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ > > | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ > > @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); > > int xfs_ilock_nowait(xfs_inode_t *, uint); > > void xfs_iunlock(xfs_inode_t *, uint); > > void xfs_ilock_demote(xfs_inode_t *, uint); > > -int xfs_isilocked(xfs_inode_t *, uint); > > +bool xfs_isilocked(struct xfs_inode *, uint); > > uint xfs_ilock_data_map_shared(struct xfs_inode *); > > uint xfs_ilock_attr_map_shared(struct xfs_inode *); > > > > -- > > 2.26.2 > > >
On Mon, Oct 12, 2020 at 02:28:18PM -0700, Darrick J. Wong wrote: > On Mon, Oct 12, 2020 at 12:03:08PM -0400, Brian Foster wrote: > > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: > > > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > > > __xfs_rwsem_islocked() is a helper function which encapsulates checking > > > state of rw_semaphores hold by inode. > > > > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > > Suggested-by: Dave Chinner <dchinner@redhat.com> > > > Suggested-by: Eric Sandeen <sandeen@redhat.com> > > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > > > fs/xfs/xfs_inode.h | 21 +++++++++++++------- > > > 2 files changed, 54 insertions(+), 15 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > > index c06129cffba9..7c1ceb4df4ec 100644 > > > --- a/fs/xfs/xfs_inode.c > > > +++ b/fs/xfs/xfs_inode.c > > > @@ -345,9 +345,43 @@ xfs_ilock_demote( > > > } > > > > > > #if defined(DEBUG) || defined(XFS_WARN) > > > -int > > > +static inline bool > > > +__xfs_rwsem_islocked( > > > + struct rw_semaphore *rwsem, > > > + int lock_flags) > > > +{ > > > + int arg; > > > + > > > + if (!debug_locks) > > > + return rwsem_is_locked(rwsem); > > > + > > > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > > > + /* > > > + * The caller could be asking if we have (shared | excl) > > > + * access to the lock. Ask lockdep if the rwsem is > > > + * locked either for read or write access. > > > + * > > > + * The caller could also be asking if we have only > > > + * shared access to the lock. Holding a rwsem > > > + * write-locked implies read access as well, so the > > > + * request to lockdep is the same for this case. > > > + */ > > > + arg = -1; > > > + } else { > > > + /* > > > + * The caller is asking if we have only exclusive access > > > + * to the lock. Ask lockdep if the rwsem is locked for > > > + * write access. > > > + */ > > > + arg = 0; > > > + } > > > > Are these arg values documented somewhere? A quick look at the function > > below didn't show anything.. > > Alas, no. :( > > If you trace lockdep_is_held_type -> lock_is_held_type -> __lock_is_held > then you'll notice that "if (read == -1" bit, but none of those > functions are documented. > Ok, so -1 is basically a catchall that causes lockdep to tell us whether the lock is held or not (in any mode). Any other value is presumably defined by the lockdep tracking infrastructure since it directly compares to ->read. Hmm.. lockdep.h has this: #define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 1, n, i) #define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 1, n, i) #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 1, n, i) ... which at least seems to correlate the values with modes (0 == exclusive, 1 || 2 == shared, etc.). I think an additional sentence or two on that in our comments would be helpful for the next person that needs to grok this code. For example, something like the following (which factors in the above two comments and IMO is slightly more clear): "If the shared flag is not set, pass 0 to explicitly check for exclusive access to the lock. If the shared flag is set, we typically want to make sure the lock is at least held in shared mode (i.e., shared | excl) but we don't necessarily care that it might actually be held exclusive. Therefore, pass -1 to check whether the lock is held in any mode rather than one of the explicit shared mode values (1 or 2)." Brian > So I have no if that's /really/ permanent, other than to say that it > exists because Dave and Christoph and I requested it years ago and > commit f8319483f57f1 has been unchanged since 2016. > > --D > > > Also, I find the pattern of shifting in the caller slightly confusing, > > particularly with the 'lock_flags' name being passed down through the > > caller. Any reason we couldn't pass the shift value as a parameter and > > do the shift at the top of the function so the logic is clear and in one > > place? > > > > > + > > > + return lockdep_is_held_type(rwsem, arg); > > > +} > > > + > > > +bool > > > xfs_isilocked( > > > - xfs_inode_t *ip, > > > + struct xfs_inode *ip, > > > uint lock_flags) > > > { > > > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { > > ... > > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > > > index e9a8bb184d1f..77776af75c77 100644 > > > --- a/fs/xfs/xfs_inode.h > > > +++ b/fs/xfs/xfs_inode.h > > > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) > > > * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) > > > * 1<<16 - 1<<32-1 -- lockdep annotation (integers) > > > */ > > > -#define XFS_IOLOCK_EXCL (1<<0) > > > -#define XFS_IOLOCK_SHARED (1<<1) > > > -#define XFS_ILOCK_EXCL (1<<2) > > > -#define XFS_ILOCK_SHARED (1<<3) > > > -#define XFS_MMAPLOCK_EXCL (1<<4) > > > -#define XFS_MMAPLOCK_SHARED (1<<5) > > > + > > > +#define XFS_IOLOCK_FLAG_SHIFT 0 > > > +#define XFS_ILOCK_FLAG_SHIFT 2 > > > +#define XFS_MMAPLOCK_FLAG_SHIFT 4 > > > + > > > +#define XFS_SHARED_LOCK_SHIFT 1 > > > + > > > +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) > > > +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > > +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) > > > +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > > +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) > > > +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) > > > > > > > Any reason for the extra params around the shift values? > > > > Brian > > > > > #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ > > > | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ > > > @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); > > > int xfs_ilock_nowait(xfs_inode_t *, uint); > > > void xfs_iunlock(xfs_inode_t *, uint); > > > void xfs_ilock_demote(xfs_inode_t *, uint); > > > -int xfs_isilocked(xfs_inode_t *, uint); > > > +bool xfs_isilocked(struct xfs_inode *, uint); > > > uint xfs_ilock_data_map_shared(struct xfs_inode *); > > > uint xfs_ilock_attr_map_shared(struct xfs_inode *); > > > > > > -- > > > 2.26.2 > > > > > >
On 10/12/20 6:03 PM, Brian Foster wrote: > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: >> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). >> __xfs_rwsem_islocked() is a helper function which encapsulates checking >> state of rw_semaphores hold by inode. >> >> Signed-off-by: Pavel Reichl <preichl@redhat.com> >> Suggested-by: Dave Chinner <dchinner@redhat.com> >> Suggested-by: Eric Sandeen <sandeen@redhat.com> >> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> >> --- >> fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- >> fs/xfs/xfs_inode.h | 21 +++++++++++++------- >> 2 files changed, 54 insertions(+), 15 deletions(-) >> >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c >> index c06129cffba9..7c1ceb4df4ec 100644 >> --- a/fs/xfs/xfs_inode.c >> +++ b/fs/xfs/xfs_inode.c >> @@ -345,9 +345,43 @@ xfs_ilock_demote( >> } >> >> #if defined(DEBUG) || defined(XFS_WARN) >> -int >> +static inline bool >> +__xfs_rwsem_islocked( >> + struct rw_semaphore *rwsem, >> + int lock_flags) >> +{ >> + int arg; >> + >> + if (!debug_locks) >> + return rwsem_is_locked(rwsem); >> + >> + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { >> + /* >> + * The caller could be asking if we have (shared | excl) >> + * access to the lock. Ask lockdep if the rwsem is >> + * locked either for read or write access. >> + * >> + * The caller could also be asking if we have only >> + * shared access to the lock. Holding a rwsem >> + * write-locked implies read access as well, so the >> + * request to lockdep is the same for this case. >> + */ >> + arg = -1; >> + } else { >> + /* >> + * The caller is asking if we have only exclusive access >> + * to the lock. Ask lockdep if the rwsem is locked for >> + * write access. >> + */ >> + arg = 0; >> + } ... > > Also, I find the pattern of shifting in the caller slightly confusing, > particularly with the 'lock_flags' name being passed down through the > caller. Any reason we couldn't pass the shift value as a parameter and > do the shift at the top of the function so the logic is clear and in one > place? > Hi Brian, is following change what you had in mind? Thanks! >> @@ -349,14 +349,16 @@ xfs_ilock_demote( static inline bool __xfs_rwsem_islocked( struct rw_semaphore *rwsem, - int lock_flags) + int lock_flags, + int shift) { int arg; + const int shifted_lock_flags = lock_flags >> shift; if (!debug_locks) return rwsem_is_locked(rwsem); - if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { + if (shifted_lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { /* * The caller could be asking if we have (shared | excl) * access to the lock. Ask lockdep if the rwsem is @@ -387,20 +389,20 @@ xfs_isilocked( { if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))); - return __xfs_rwsem_islocked(&ip->i_lock, - (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); + return __xfs_rwsem_islocked(&ip->i_lock, lock_flags, + XFS_ILOCK_FLAG_SHIFT); } if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { ASSERT(!(lock_flags & ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))); - return __xfs_rwsem_islocked(&ip->i_mmaplock, - (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); + return __xfs_rwsem_islocked(&ip->i_mmaplock, lock_flags, + XFS_MMAPLOCK_FLAG_SHIFT); } if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { - return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, - (lock_flags >> XFS_IOLOCK_FLAG_SHIFT)); + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, lock_flags, + XFS_IOLOCK_FLAG_SHIFT); } ASSERT(0);
On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > __xfs_rwsem_islocked() is a helper function which encapsulates checking > state of rw_semaphores hold by inode. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > Suggested-by: Dave Chinner <dchinner@redhat.com> > Suggested-by: Eric Sandeen <sandeen@redhat.com> > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > fs/xfs/xfs_inode.h | 21 +++++++++++++------- > 2 files changed, 54 insertions(+), 15 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c06129cffba9..7c1ceb4df4ec 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -345,9 +345,43 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + int lock_flags) > +{ > + int arg; > + > + if (!debug_locks) > + return rwsem_is_locked(rwsem); > + > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + /* > + * The caller could be asking if we have (shared | excl) > + * access to the lock. Ask lockdep if the rwsem is > + * locked either for read or write access. > + * > + * The caller could also be asking if we have only > + * shared access to the lock. Holding a rwsem > + * write-locked implies read access as well, so the > + * request to lockdep is the same for this case. > + */ > + arg = -1; > + } else { > + /* > + * The caller is asking if we have only exclusive access > + * to the lock. Ask lockdep if the rwsem is locked for > + * write access. > + */ > + arg = 0; > + } > + > + return lockdep_is_held_type(rwsem, arg); Why not write this as: if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { ... return lockdep_is_held_type(rwsem, 1); } ... return lockdep_is_held_type(rwsem, 0); } which seems a lot easier to read compare to the strange arg variable. Otherwise this looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Oct 14, 2020 at 11:04:31PM +0200, Pavel Reichl wrote: > > > On 10/12/20 6:03 PM, Brian Foster wrote: > > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote: > >> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked(). > >> __xfs_rwsem_islocked() is a helper function which encapsulates checking > >> state of rw_semaphores hold by inode. > >> > >> Signed-off-by: Pavel Reichl <preichl@redhat.com> > >> Suggested-by: Dave Chinner <dchinner@redhat.com> > >> Suggested-by: Eric Sandeen <sandeen@redhat.com> > >> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > >> --- > >> fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > >> fs/xfs/xfs_inode.h | 21 +++++++++++++------- > >> 2 files changed, 54 insertions(+), 15 deletions(-) > >> > >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > >> index c06129cffba9..7c1ceb4df4ec 100644 > >> --- a/fs/xfs/xfs_inode.c > >> +++ b/fs/xfs/xfs_inode.c > >> @@ -345,9 +345,43 @@ xfs_ilock_demote( > >> } > >> > >> #if defined(DEBUG) || defined(XFS_WARN) > >> -int > >> +static inline bool > >> +__xfs_rwsem_islocked( > >> + struct rw_semaphore *rwsem, > >> + int lock_flags) > >> +{ > >> + int arg; > >> + > >> + if (!debug_locks) > >> + return rwsem_is_locked(rwsem); > >> + > >> + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > >> + /* > >> + * The caller could be asking if we have (shared | excl) > >> + * access to the lock. Ask lockdep if the rwsem is > >> + * locked either for read or write access. > >> + * > >> + * The caller could also be asking if we have only > >> + * shared access to the lock. Holding a rwsem > >> + * write-locked implies read access as well, so the > >> + * request to lockdep is the same for this case. > >> + */ > >> + arg = -1; > >> + } else { > >> + /* > >> + * The caller is asking if we have only exclusive access > >> + * to the lock. Ask lockdep if the rwsem is locked for > >> + * write access. > >> + */ > >> + arg = 0; > >> + } > ... > > > > Also, I find the pattern of shifting in the caller slightly confusing, > > particularly with the 'lock_flags' name being passed down through the > > caller. Any reason we couldn't pass the shift value as a parameter and > > do the shift at the top of the function so the logic is clear and in one > > place? > > > > Hi Brian, is following change what you had in mind? Thanks! > Yep, pretty much. I find shifted_lock_flags to be a little verbose as a name. I'd be fine with just doing something like 'lock_flags >>= shift' near the top of the function, but that's more of a personal nit. I also like Christoph's suggestion to avoid the arg variable (along with the comment update suggested in the discussion with Darrick). Brian > > >> @@ -349,14 +349,16 @@ xfs_ilock_demote( > static inline bool > __xfs_rwsem_islocked( > struct rw_semaphore *rwsem, > - int lock_flags) > + int lock_flags, > + int shift) > { > int arg; > + const int shifted_lock_flags = lock_flags >> shift; > > if (!debug_locks) > return rwsem_is_locked(rwsem); > > - if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + if (shifted_lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > /* > * The caller could be asking if we have (shared | excl) > * access to the lock. Ask lockdep if the rwsem is > @@ -387,20 +389,20 @@ xfs_isilocked( > { > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { > ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))); > - return __xfs_rwsem_islocked(&ip->i_lock, > - (lock_flags >> XFS_ILOCK_FLAG_SHIFT)); > + return __xfs_rwsem_islocked(&ip->i_lock, lock_flags, > + XFS_ILOCK_FLAG_SHIFT); > } > > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { > ASSERT(!(lock_flags & > ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))); > - return __xfs_rwsem_islocked(&ip->i_mmaplock, > - (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT)); > + return __xfs_rwsem_islocked(&ip->i_mmaplock, lock_flags, > + XFS_MMAPLOCK_FLAG_SHIFT); > } > > if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { > - return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, > - (lock_flags >> XFS_IOLOCK_FLAG_SHIFT)); > + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, lock_flags, > + XFS_IOLOCK_FLAG_SHIFT); > } > > ASSERT(0); >
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index c06129cffba9..7c1ceb4df4ec 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -345,9 +345,43 @@ xfs_ilock_demote( } #if defined(DEBUG) || defined(XFS_WARN) -int +static inline bool +__xfs_rwsem_islocked( + struct rw_semaphore *rwsem, + int lock_flags) +{ + int arg; + + if (!debug_locks) + return rwsem_is_locked(rwsem); + + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { + /* + * The caller could be asking if we have (shared | excl) + * access to the lock. Ask lockdep if the rwsem is + * locked either for read or write access. + * + * The caller could also be asking if we have only + * shared access to the lock. Holding a rwsem + * write-locked implies read access as well, so the + * request to lockdep is the same for this case. + */ + arg = -1; + } else { + /* + * The caller is asking if we have only exclusive access + * to the lock. Ask lockdep if the rwsem is locked for + * write access. + */ + arg = 0; + } + + return lockdep_is_held_type(rwsem, arg); +} + +bool xfs_isilocked( - xfs_inode_t *ip, + struct xfs_inode *ip, uint lock_flags) { if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { @@ -362,15 +396,13 @@ xfs_isilocked( return rwsem_is_locked(&ip->i_mmaplock.mr_lock); } - if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) { - if (!(lock_flags & XFS_IOLOCK_SHARED)) - return !debug_locks || - lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0); - return rwsem_is_locked(&VFS_I(ip)->i_rwsem); + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, + (lock_flags >> XFS_IOLOCK_FLAG_SHIFT)); } ASSERT(0); - return 0; + return false; } #endif diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index e9a8bb184d1f..77776af75c77 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) * Bit ranges: 1<<1 - 1<<16-1 -- iolock/ilock modes (bitfield) * 1<<16 - 1<<32-1 -- lockdep annotation (integers) */ -#define XFS_IOLOCK_EXCL (1<<0) -#define XFS_IOLOCK_SHARED (1<<1) -#define XFS_ILOCK_EXCL (1<<2) -#define XFS_ILOCK_SHARED (1<<3) -#define XFS_MMAPLOCK_EXCL (1<<4) -#define XFS_MMAPLOCK_SHARED (1<<5) + +#define XFS_IOLOCK_FLAG_SHIFT 0 +#define XFS_ILOCK_FLAG_SHIFT 2 +#define XFS_MMAPLOCK_FLAG_SHIFT 4 + +#define XFS_SHARED_LOCK_SHIFT 1 + +#define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_FLAG_SHIFT)) +#define XFS_IOLOCK_SHARED (XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) +#define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_FLAG_SHIFT)) +#define XFS_ILOCK_SHARED (XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) +#define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_FLAG_SHIFT)) +#define XFS_MMAPLOCK_SHARED (XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT)) #define XFS_LOCK_MASK (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \ | XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \ @@ -412,7 +419,7 @@ void xfs_ilock(xfs_inode_t *, uint); int xfs_ilock_nowait(xfs_inode_t *, uint); void xfs_iunlock(xfs_inode_t *, uint); void xfs_ilock_demote(xfs_inode_t *, uint); -int xfs_isilocked(xfs_inode_t *, uint); +bool xfs_isilocked(struct xfs_inode *, uint); uint xfs_ilock_data_map_shared(struct xfs_inode *); uint xfs_ilock_attr_map_shared(struct xfs_inode *);