| Message ID | 20200227203636.317790-2-preichl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: Remove wrappers for some semaphores | expand |
On Thu, Feb 27, 2020 at 09:36:33PM +0100, 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> > --- > Changes from V5: > Drop shared flag from __xfs_rwsem_islocked() > > > fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++---------------- > fs/xfs/xfs_inode.h | 2 +- > 2 files changed, 27 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c5077e6326c7..4faf7827717b 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -345,32 +345,42 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + bool excl) > +{ > + if (!rwsem_is_locked(rwsem)) > + return false; So, uh, I finally made the time to dig through what exactly lockdep provides as far as testing functions, and came up with the following truth table for the old xfs_isilocked behavior w.r.t. IOLOCK: (nolockdep corresponds to debug_locks == 0) RWSEM STATE PARAMETERS TO XFS_ISILOCKED: SHARED EXCL SHARED | EXCL readlocked y n y writelocked y y y unlocked n n n nolockdep readlocked y y y nolockdep writelocked y y y nolockdep unlocked n y n Note that EXCL × nolockdep_unlocked returns an incorrect result, but because we only use it with ASSERTs there haven't been any failures reported. And here's your new version: readlocked y y y writelocked y n n unlocked n n n nolockdep readlocked y y y nolockdep writelocked y y y nolockdep unlocked n n n Thanks for fixing the false positive that I mentioned above. > + > + if (debug_locks && excl) > + return lockdep_is_held_type(rwsem, 1); This is wrong, the second parameter of lockdep_is_held_type is 0 to test if the rwsem is write-locked; 1 to test if it is read-locked; or -1 to test if the rwsem is read or write-locked. So, this function's call signature should change so that callers can communicate both _SHARED and _EXCL; and then you can pick the correct "r" parameter value for the lockdep_is_held_type() call. Then all of this becomes: if !debug_locks: return rwsem_is_locked(rwsem) if shared and excl: r = -1 elif shared: r = 1 else: r = 0 return lockdep_is_held_type(rwsem, r) Note also that you don't necessarily need to pass shared and excl as separate parameters (as you did in v3); the XFS_*LOCK_{EXCL,SHARED} definitions enable you to take care of all that with some clever bit shifting and masking. --D > + > + return true; > +} > + > +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.mr_lock, > + (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); > + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { > + return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock, > + (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); > + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { > + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, > + (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 *); > > -- > 2.24.1 >
On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Feb 27, 2020 at 09:36:33PM +0100, 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> > > --- > > Changes from V5: > > Drop shared flag from __xfs_rwsem_islocked() > > > > > > fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++---------------- > > fs/xfs/xfs_inode.h | 2 +- > > 2 files changed, 27 insertions(+), 17 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index c5077e6326c7..4faf7827717b 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -345,32 +345,42 @@ xfs_ilock_demote( > > } > > > > #if defined(DEBUG) || defined(XFS_WARN) > > -int > > +static inline bool > > +__xfs_rwsem_islocked( > > + struct rw_semaphore *rwsem, > > + bool excl) > > +{ > > + if (!rwsem_is_locked(rwsem)) > > + return false; > > So, uh, I finally made the time to dig through what exactly lockdep > provides as far as testing functions, and came up with the following > truth table for the old xfs_isilocked behavior w.r.t. IOLOCK: Thank you for doing that and I'm sorry that I'm replying with such a delay. > > (nolockdep corresponds to debug_locks == 0) > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > SHARED EXCL SHARED | EXCL > readlocked y n y > writelocked y y y > unlocked n n n > nolockdep readlocked y y y > nolockdep writelocked y y y > nolockdep unlocked n y n > > Note that EXCL × nolockdep_unlocked returns an incorrect result, but > because we only use it with ASSERTs there haven't been any failures > reported. > > And here's your new version: > > readlocked y y y > writelocked y n n > unlocked n n n > nolockdep readlocked y y y > nolockdep writelocked y y y > nolockdep unlocked n n n > > Thanks for fixing the false positive that I mentioned above. > > > + > > + if (debug_locks && excl) > > + return lockdep_is_held_type(rwsem, 1); > > This is wrong, the second parameter of lockdep_is_held_type is 0 to test > if the rwsem is write-locked; 1 to test if it is read-locked; or -1 to > test if the rwsem is read or write-locked. Thank you for noticing. > > So, this function's call signature should change so that callers can > communicate both _SHARED and _EXCL; and then you can pick the correct Thanks for the suggestion...but that's how v5 signature looked like before Christoph and Eric requested change...on the grounds that there're: * confusion over a (true, true) set of args * confusion of what happens if we pass (false, false). > "r" parameter value for the lockdep_is_held_type() call. Then all of > this becomes: > > if !debug_locks: > return rwsem_is_locked(rwsem) > > if shared and excl: > r = -1 > elif shared: > r = 1 > else: > r = 0 > return lockdep_is_held_type(rwsem, r) I tried to create a table for this code as well: readlocked y n y writelocked *n* y y unlocked n n n nolockdep readlocked y y y nolockdep writelocked y y y nolockdep unlocked n n n I think that when we query writelocked lock for being shared having 'no' for an answer may not be expected...or at least this is how I read the code. int __lock_is_held(const struct lockdep_map *lock, int read) { struct task_struct *curr = current; int i; for (i = 0; i < curr->lockdep_depth; i++) { struct held_lock *hlock = curr->held_locks + i; if (match_held_lock(hlock, lock)) { if (read == -1 || hlock->read == read) return 1; return 0; } } return 0; } Thanks for any comments > > Note also that you don't necessarily need to pass shared and excl as > separate parameters (as you did in v3); the XFS_*LOCK_{EXCL,SHARED} > definitions enable you to take care of all that with some clever bit > shifting and masking. > > --D > > > + > > + return true; > > +} > > + > > +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.mr_lock, > > + (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); > > + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { > > + return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock, > > + (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); > > + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { > > + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, > > + (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 *); > > > > -- > > 2.24.1 > > >
On 3/18/20 12:13 PM, Pavel Reichl wrote: > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: ... >> So, this function's call signature should change so that callers can >> communicate both _SHARED and _EXCL; and then you can pick the correct > > Thanks for the suggestion...but that's how v5 signature looked like > before Christoph and Eric requested change...on the grounds that > there're: > * confusion over a (true, true) set of args > * confusion of what happens if we pass (false, false). > >> "r" parameter value for the lockdep_is_held_type() call. Then all of >> this becomes: >> >> if !debug_locks: >> return rwsem_is_locked(rwsem) >> >> if shared and excl: >> r = -1 >> elif shared: >> r = 1 >> else: >> r = 0 >> return lockdep_is_held_type(rwsem, r) > > I tried to create a table for this code as well: <adding back the table headers> > (nolockdep corresponds to debug_locks == 0) > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > SHARED EXCL SHARED | EXCL > readlocked y n y > writelocked *n* y y > unlocked n n n > nolockdep readlocked y y y > nolockdep writelocked y y y > nolockdep unlocked n n n > > I think that when we query writelocked lock for being shared having > 'no' for an answer may not be expected...or at least this is how I > read the code. This might be ok, because a) it is technically correct (is it shared? /no/ it is exclusive), and b) in the XFS code today we never call: xfs_isilocked(ip, XFS_ILOCK_SHARED); it's always: xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); So I think that if we document the behavior clearly, the truth table above would be ok. Thoughts? -Eric
On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote: > On 3/18/20 12:13 PM, Pavel Reichl wrote: > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > ... > > >> So, this function's call signature should change so that callers can > >> communicate both _SHARED and _EXCL; and then you can pick the correct > > > > Thanks for the suggestion...but that's how v5 signature looked like > > before Christoph and Eric requested change...on the grounds that > > there're: > > * confusion over a (true, true) set of args > > * confusion of what happens if we pass (false, false). Yeah. I don't mean adding back the dual booleans, I meant refactoring the way we define the lock constants so that you can use bit shifting and masking: #define XFS_IOLOCK_SHIFT 0 #define XFS_ILOCK_SHIFT 2 #define XFS_MMAPLOCK_SHIFT 4 #define XFS_SHARED_LOCK_SHIFT 1 #define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_SHIFT)) #define XFS_IOLOCK_SHARED (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) #define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_SHIFT)) #define XFS_ILOCK_SHARED (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) #define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_SHIFT)) #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) Because then in the outer xfs_isilocked function you can do: if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT); if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT); if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT); And finally in __isilocked you can do: static inline bool __isilocked(rwsem, 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); } > >> "r" parameter value for the lockdep_is_held_type() call. Then all of > >> this becomes: > >> > >> if !debug_locks: > >> return rwsem_is_locked(rwsem) > >> > >> if shared and excl: > >> r = -1 > >> elif shared: > >> r = 1 > >> else: > >> r = 0 > >> return lockdep_is_held_type(rwsem, r) > > > > I tried to create a table for this code as well: > > <adding back the table headers> > > > (nolockdep corresponds to debug_locks == 0) > > > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > > SHARED EXCL SHARED | EXCL > > readlocked y n y > > writelocked *n* y y > > unlocked n n n > > nolockdep readlocked y y y > > nolockdep writelocked y y y > > nolockdep unlocked n n n > > > > I think that when we query writelocked lock for being shared having > > 'no' for an answer may not be expected...or at least this is how I > > read the code. > > This might be ok, because > a) it is technically correct (is it shared? /no/ it is exclusive), and > b) in the XFS code today we never call: > > xfs_isilocked(ip, XFS_ILOCK_SHARED); > > it's always: > > xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); > > So I think that if we document the behavior clearly, the truth table above > would be ok. > > Thoughts? No, Pavel's right, I got the pseudocode wrong, because holding a write lock means you also hold the read lock. if !debug_locks: return rwsem_is_locked(rwsem) if shared: r = -1 else: r = 0 return lockdep_is_held_type(rwsem, r) --D > -Eric
On 3/18/20 1:49 PM, Darrick J. Wong wrote: >>> I think that when we query writelocked lock for being shared having >>> 'no' for an answer may not be expected...or at least this is how I >>> read the code. >> This might be ok, because >> a) it is technically correct (is it shared? /no/ it is exclusive), and >> b) in the XFS code today we never call: >> >> xfs_isilocked(ip, XFS_ILOCK_SHARED); >> >> it's always: >> >> xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); >> >> So I think that if we document the behavior clearly, the truth table above >> would be ok. >> >> Thoughts? > No, Pavel's right, I got the pseudocode wrong, because holding a write > lock means you also hold the read lock. > > if !debug_locks: > return rwsem_is_locked(rwsem) > > if shared: > r = -1 > else: > r = 0 > return lockdep_is_held_type(rwsem, r) I think it comes down to semantics, and either way is fine as long as we don't change the behavior of existing callers, and document exactly what the islocked function does now. -Eric
On Wed, Mar 18, 2020 at 7:50 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote: > > On 3/18/20 12:13 PM, Pavel Reichl wrote: > > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > ... > > > > >> So, this function's call signature should change so that callers can > > >> communicate both _SHARED and _EXCL; and then you can pick the correct > > > > > > Thanks for the suggestion...but that's how v5 signature looked like > > > before Christoph and Eric requested change...on the grounds that > > > there're: > > > * confusion over a (true, true) set of args > > > * confusion of what happens if we pass (false, false). > > Yeah. I don't mean adding back the dual booleans, I meant refactoring > the way we define the lock constants so that you can use bit shifting > and masking: > > #define XFS_IOLOCK_SHIFT 0 > #define XFS_ILOCK_SHIFT 2 > #define XFS_MMAPLOCK_SHIFT 4 > > #define XFS_SHARED_LOCK_SHIFT 1 > > #define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_SHIFT)) > #define XFS_IOLOCK_SHARED (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) > #define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_SHIFT)) > #define XFS_ILOCK_SHARED (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) > #define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_SHIFT)) > #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) Thank you for the code - now I see what you meant and I like it, however allow me a question: Are you aware that XFS_IOLOCK_SHIFT, XFS_MMAPLOCK_SHIFT, XFS_ILOCK_SHIFT are already defined with different values and used in xfs_lock_inumorder()? I have no trouble to investigate the code and see if it is OK i.g. XFS_IOLOCK_EXCL to be 21 (I guess I should check that no bit arrays are used to store the value, etc) Or maybe I should just rewrite the '#define XFS_IOLOCK_SHIFT 0' to something like '#define XFS_IOLOCK_TYPE_SHIFT 0' ? Do you have any thoughts about that? Thanks! > > Because then in the outer xfs_isilocked function you can do: > > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) > return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT); > > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) > return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT); > > if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) > return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT); > > And finally in __isilocked you can do: > > static inline bool __isilocked(rwsem, 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); > } > > > >> "r" parameter value for the lockdep_is_held_type() call. Then all of > > >> this becomes: > > >> > > >> if !debug_locks: > > >> return rwsem_is_locked(rwsem) > > >> > > >> if shared and excl: > > >> r = -1 > > >> elif shared: > > >> r = 1 > > >> else: > > >> r = 0 > > >> return lockdep_is_held_type(rwsem, r) > > > > > > I tried to create a table for this code as well: > > > > <adding back the table headers> > > > > > (nolockdep corresponds to debug_locks == 0) > > > > > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > > > SHARED EXCL SHARED | EXCL > > > readlocked y n y > > > writelocked *n* y y > > > unlocked n n n > > > nolockdep readlocked y y y > > > nolockdep writelocked y y y > > > nolockdep unlocked n n n > > > > > > I think that when we query writelocked lock for being shared having > > > 'no' for an answer may not be expected...or at least this is how I > > > read the code. > > > > This might be ok, because > > a) it is technically correct (is it shared? /no/ it is exclusive), and > > b) in the XFS code today we never call: > > > > xfs_isilocked(ip, XFS_ILOCK_SHARED); > > > > it's always: > > > > xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); > > > > So I think that if we document the behavior clearly, the truth table above > > would be ok. > > > > Thoughts? > > No, Pavel's right, I got the pseudocode wrong, because holding a write > lock means you also hold the read lock. > > if !debug_locks: > return rwsem_is_locked(rwsem) > > if shared: > r = -1 > else: > r = 0 > return lockdep_is_held_type(rwsem, r) > > --D > > > -Eric >
On Fri, Mar 20, 2020 at 03:41:08PM +0100, Pavel Reichl wrote: > On Wed, Mar 18, 2020 at 7:50 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote: > > > On 3/18/20 12:13 PM, Pavel Reichl wrote: > > > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > > > ... > > > > > > >> So, this function's call signature should change so that callers can > > > >> communicate both _SHARED and _EXCL; and then you can pick the correct > > > > > > > > Thanks for the suggestion...but that's how v5 signature looked like > > > > before Christoph and Eric requested change...on the grounds that > > > > there're: > > > > * confusion over a (true, true) set of args > > > > * confusion of what happens if we pass (false, false). > > > > Yeah. I don't mean adding back the dual booleans, I meant refactoring > > the way we define the lock constants so that you can use bit shifting > > and masking: > > > > #define XFS_IOLOCK_SHIFT 0 > > #define XFS_ILOCK_SHIFT 2 > > #define XFS_MMAPLOCK_SHIFT 4 > > > > #define XFS_SHARED_LOCK_SHIFT 1 > > > > #define XFS_IOLOCK_EXCL (1 << (XFS_IOLOCK_SHIFT)) > > #define XFS_IOLOCK_SHARED (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) > > #define XFS_ILOCK_EXCL (1 << (XFS_ILOCK_SHIFT)) > > #define XFS_ILOCK_SHARED (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) > > #define XFS_MMAPLOCK_EXCL (1 << (XFS_MMAPLOCK_SHIFT)) > > #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT)) > > Thank you for the code - now I see what you meant and I like it, > however allow me a question: > Are you aware that XFS_IOLOCK_SHIFT, XFS_MMAPLOCK_SHIFT, > XFS_ILOCK_SHIFT are already defined with different values and used in > xfs_lock_inumorder()? > > I have no trouble to investigate the code and see if it is OK i.g. > XFS_IOLOCK_EXCL to be 21 (I guess I should check that no bit arrays > are used to store the value, etc) > > Or maybe I should just rewrite the '#define XFS_IOLOCK_SHIFT > 0' to something like '#define XFS_IOLOCK_TYPE_SHIFT 0' ? > > Do you have any thoughts about that? XFS_IOLOCK_TYPE_SHIFT seems fine to me to avoid clashing with lockdep. :) (perhaps XFS_IOLOCK_FLAG_SHIFT?) --D > > Thanks! > > > > > > Because then in the outer xfs_isilocked function you can do: > > > > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) > > return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT); > > > > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) > > return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT); > > > > if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) > > return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT); > > > > And finally in __isilocked you can do: > > > > static inline bool __isilocked(rwsem, 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); > > } > > > > > >> "r" parameter value for the lockdep_is_held_type() call. Then all of > > > >> this becomes: > > > >> > > > >> if !debug_locks: > > > >> return rwsem_is_locked(rwsem) > > > >> > > > >> if shared and excl: > > > >> r = -1 > > > >> elif shared: > > > >> r = 1 > > > >> else: > > > >> r = 0 > > > >> return lockdep_is_held_type(rwsem, r) > > > > > > > > I tried to create a table for this code as well: > > > > > > <adding back the table headers> > > > > > > > (nolockdep corresponds to debug_locks == 0) > > > > > > > > RWSEM STATE PARAMETERS TO XFS_ISILOCKED: > > > > SHARED EXCL SHARED | EXCL > > > > readlocked y n y > > > > writelocked *n* y y > > > > unlocked n n n > > > > nolockdep readlocked y y y > > > > nolockdep writelocked y y y > > > > nolockdep unlocked n n n > > > > > > > > I think that when we query writelocked lock for being shared having > > > > 'no' for an answer may not be expected...or at least this is how I > > > > read the code. > > > > > > This might be ok, because > > > a) it is technically correct (is it shared? /no/ it is exclusive), and > > > b) in the XFS code today we never call: > > > > > > xfs_isilocked(ip, XFS_ILOCK_SHARED); > > > > > > it's always: > > > > > > xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL); > > > > > > So I think that if we document the behavior clearly, the truth table above > > > would be ok. > > > > > > Thoughts? > > > > No, Pavel's right, I got the pseudocode wrong, because holding a write > > lock means you also hold the read lock. > > > > if !debug_locks: > > return rwsem_is_locked(rwsem) > > > > if shared: > > r = -1 > > else: > > r = 0 > > return lockdep_is_held_type(rwsem, r) > > > > --D > > > > > -Eric > > >
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index c5077e6326c7..4faf7827717b 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -345,32 +345,42 @@ xfs_ilock_demote( } #if defined(DEBUG) || defined(XFS_WARN) -int +static inline bool +__xfs_rwsem_islocked( + struct rw_semaphore *rwsem, + bool excl) +{ + if (!rwsem_is_locked(rwsem)) + return false; + + if (debug_locks && excl) + return lockdep_is_held_type(rwsem, 1); + + return true; +} + +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.mr_lock, + (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); + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { + return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock, + (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); + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, + (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 *);
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> --- Changes from V5: Drop shared flag from __xfs_rwsem_islocked() fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++---------------- fs/xfs/xfs_inode.h | 2 +- 2 files changed, 27 insertions(+), 17 deletions(-)