| Message ID | 20200214185942.1147742-1-preichl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v5,1/4] xfs: Refactor xfs_isilocked() | expand |
Since it has more than one patch and version 5, I couldn't find the cover-letter and a change log for this series, is there a particular reason why it is not present or I missed it? On 02/14/2020 11:00 AM, 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. > >
On Fri, Feb 14, 2020 at 07:59:39PM +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> > --- > fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++-------------- > fs/xfs/xfs_inode.h | 2 +- > 2 files changed, 39 insertions(+), 17 deletions(-) Looks good. Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Sat, Feb 15, 2020 at 2:38 AM Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> wrote: > > Since it has more than one patch and version 5, > I couldn't find the cover-letter and a change log for this > series, is there a particular reason why it is not present or I > missed it? > > On 02/14/2020 11:00 AM, 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. > > > > > > Hi Chaitanya, sorry for the absence of the changelog I forgot to add it - that was not on purpose. To summarize the changes: I moved the asserts from the first patch to the third as suggested by Eric and changed the commit messages as suggested by Dave. Regarding the missing cover-letter it was same since version one and I was not sure I should resend it with every new version, should I? Thanks you for your comments. Best regards Pavel Reichl
On Fri, Feb 14, 2020 at 07:59:39PM +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> > --- > fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++-------------- > fs/xfs/xfs_inode.h | 2 +- > 2 files changed, 39 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c5077e6326c7..3d28c4790231 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -345,32 +345,54 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + bool shared, > + bool excl) > +{ > + bool locked = false; > + > + if (!rwsem_is_locked(rwsem)) > + return false; > + > + if (!debug_locks) > + return true; > + > + if (shared) > + locked = lockdep_is_held_type(rwsem, 0); > + > + if (excl) > + locked |= lockdep_is_held_type(rwsem, 1); > + > + return locked; This could use some comments explaining the logic, especially why we need the shared and excl flags, which seems very confusing given that a lock can be held either shared or exclusive, but not neither and not both.
On Mon, Feb 17, 2020 at 05:35:21AM -0800, Christoph Hellwig wrote: > On Fri, Feb 14, 2020 at 07:59:39PM +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> > > --- > > fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++-------------- > > fs/xfs/xfs_inode.h | 2 +- > > 2 files changed, 39 insertions(+), 17 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index c5077e6326c7..3d28c4790231 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -345,32 +345,54 @@ xfs_ilock_demote( > > } > > > > #if defined(DEBUG) || defined(XFS_WARN) > > -int > > +static inline bool > > +__xfs_rwsem_islocked( > > + struct rw_semaphore *rwsem, > > + bool shared, > > + bool excl) > > +{ > > + bool locked = false; > > + > > + if (!rwsem_is_locked(rwsem)) > > + return false; > > + > > + if (!debug_locks) > > + return true; > > + > > + if (shared) > > + locked = lockdep_is_held_type(rwsem, 0); > > + > > + if (excl) > > + locked |= lockdep_is_held_type(rwsem, 1); > > + > > + return locked; > > This could use some comments explaining the logic, especially why we > need the shared and excl flags, which seems very confusing given that > a lock can be held either shared or exclusive, but not neither and not > both. Yes, this predicate should document that callers are allowed to pass in shared==true and excl==true when the caller wants to assert that either lock type (shared or excl) of a given lock class (e.g. iolock) are held. --D
On Wed, Feb 19, 2020 at 5:48 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Mon, Feb 17, 2020 at 05:35:21AM -0800, Christoph Hellwig wrote: > > On Fri, Feb 14, 2020 at 07:59:39PM +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> > > > --- > > > fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++-------------- > > > fs/xfs/xfs_inode.h | 2 +- > > > 2 files changed, 39 insertions(+), 17 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > > index c5077e6326c7..3d28c4790231 100644 > > > --- a/fs/xfs/xfs_inode.c > > > +++ b/fs/xfs/xfs_inode.c > > > @@ -345,32 +345,54 @@ xfs_ilock_demote( > > > } > > > > > > #if defined(DEBUG) || defined(XFS_WARN) > > > -int > > > +static inline bool > > > +__xfs_rwsem_islocked( > > > + struct rw_semaphore *rwsem, > > > + bool shared, > > > + bool excl) > > > +{ > > > + bool locked = false; > > > + > > > + if (!rwsem_is_locked(rwsem)) > > > + return false; > > > + > > > + if (!debug_locks) > > > + return true; > > > + > > > + if (shared) > > > + locked = lockdep_is_held_type(rwsem, 0); > > > + > > > + if (excl) > > > + locked |= lockdep_is_held_type(rwsem, 1); > > > + > > > + return locked; > > > > This could use some comments explaining the logic, especially why we > > need the shared and excl flags, which seems very confusing given that > > a lock can be held either shared or exclusive, but not neither and not > > both. > > Yes, this predicate should document that callers are allowed to pass in > shared==true and excl==true when the caller wants to assert that either > lock type (shared or excl) of a given lock class (e.g. iolock) are held. > > --D > Hello, thanks for the comments. Would code comment preceding the definition of __xfs_rwsem_islocked() work for you? Something like: /* This is a helper function that encapsulates checking the state of * rw semaphores. * * if shared == true AND excl == true then function returns true if either * lock type (shared or excl) of a given semaphore are held. */
On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote: > > > +static inline bool > > > +__xfs_rwsem_islocked( > > > + struct rw_semaphore *rwsem, > > > + bool shared, > > > + bool excl) > > > +{ > > > + bool locked = false; > > > + > > > + if (!rwsem_is_locked(rwsem)) > > > + return false; > > > + > > > + if (!debug_locks) > > > + return true; > > > + > > > + if (shared) > > > + locked = lockdep_is_held_type(rwsem, 0); > > > + > > > + if (excl) > > > + locked |= lockdep_is_held_type(rwsem, 1); > > > + > > > + return locked; > > > > This could use some comments explaining the logic, especially why we > > need the shared and excl flags, which seems very confusing given that > > a lock can be held either shared or exclusive, but not neither and not > > both. > > Yes, this predicate should document that callers are allowed to pass in > shared==true and excl==true when the caller wants to assert that either > lock type (shared or excl) of a given lock class (e.g. iolock) are held. Looking at the lockdep_is_held_type implementation, and our existing code for i_rwsem I really don't see the point of the extra shared check. Something like: static inline bool __xfs_rwsem_islocked( struct rw_semaphore *rwsem, bool excl) { if (rwsem_is_locked(rwsem)) { if (debug_locks && excl) return lockdep_is_held_type(rwsem, 1); return true; } return false; } should be all that we really need.
On 2/19/20 12:40 PM, Christoph Hellwig wrote: > On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote: >>>> +static inline bool >>>> +__xfs_rwsem_islocked( >>>> + struct rw_semaphore *rwsem, >>>> + bool shared, >>>> + bool excl) >>>> +{ >>>> + bool locked = false; >>>> + >>>> + if (!rwsem_is_locked(rwsem)) >>>> + return false; >>>> + >>>> + if (!debug_locks) >>>> + return true; >>>> + >>>> + if (shared) >>>> + locked = lockdep_is_held_type(rwsem, 0); >>>> + >>>> + if (excl) >>>> + locked |= lockdep_is_held_type(rwsem, 1); >>>> + >>>> + return locked; >>> >>> This could use some comments explaining the logic, especially why we >>> need the shared and excl flags, which seems very confusing given that >>> a lock can be held either shared or exclusive, but not neither and not >>> both. >> >> Yes, this predicate should document that callers are allowed to pass in >> shared==true and excl==true when the caller wants to assert that either >> lock type (shared or excl) of a given lock class (e.g. iolock) are held. > > Looking at the lockdep_is_held_type implementation, and our existing > code for i_rwsem I really don't see the point of the extra shared > check. Something like: > > static inline bool > __xfs_rwsem_islocked( > struct rw_semaphore *rwsem, > bool excl) > { > if (rwsem_is_locked(rwsem)) { > if (debug_locks && excl) > return lockdep_is_held_type(rwsem, 1); > return true; > } > > return false; > } > > should be all that we really need. I think that's a lot more clear. In addition to the slight confusion over a (true, true) set of args, the current proposal also has the extra confusion of what happens if we pass (false, false), for example. One other thought, since debug_locks getting turned off by lockdep means that an exclusive test reverts to a shared|exclusive test, would it be worth adding a WARN_ON_ONCE to make it clear when xfs rwsem lock testing coverage has been reduced? -Eric
On 02/17/2020 02:56 AM, Pavel Reichl wrote: > On Sat, Feb 15, 2020 at 2:38 AM Chaitanya Kulkarni > <Chaitanya.Kulkarni@wdc.com> wrote: >> > >> >Since it has more than one patch and version 5, >> >I couldn't find the cover-letter and a change log for this >> >series, is there a particular reason why it is not present or I >> >missed it? >> > >> >On 02/14/2020 11:00 AM, 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. >>> > > >>> > > >> > >> > > Hi Chaitanya, > > sorry for the absence of the changelog I forgot to add it - that was > not on purpose. > > To summarize the changes: I moved the asserts from the first patch to > the third as suggested by Eric and changed the commit messages as > suggested by Dave. Thanks. > > Regarding the missing cover-letter it was same since version one and I > was not sure I should resend it with every new version, should I? It's okay, it just makes re-viewer's life easier to look for the change. > > Thanks you for your comments. > > Best regards > > Pavel Reichl > >
On Wed, Feb 19, 2020 at 9:16 PM Eric Sandeen <sandeen@sandeen.net> wrote: > > > > On 2/19/20 12:40 PM, Christoph Hellwig wrote: > > On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote: > >>>> +static inline bool > >>>> +__xfs_rwsem_islocked( > >>>> + struct rw_semaphore *rwsem, > >>>> + bool shared, > >>>> + bool excl) > >>>> +{ > >>>> + bool locked = false; > >>>> + > >>>> + if (!rwsem_is_locked(rwsem)) > >>>> + return false; > >>>> + > >>>> + if (!debug_locks) > >>>> + return true; > >>>> + > >>>> + if (shared) > >>>> + locked = lockdep_is_held_type(rwsem, 0); > >>>> + > >>>> + if (excl) > >>>> + locked |= lockdep_is_held_type(rwsem, 1); > >>>> + > >>>> + return locked; > >>> > >>> This could use some comments explaining the logic, especially why we > >>> need the shared and excl flags, which seems very confusing given that > >>> a lock can be held either shared or exclusive, but not neither and not > >>> both. > >> > >> Yes, this predicate should document that callers are allowed to pass in > >> shared==true and excl==true when the caller wants to assert that either > >> lock type (shared or excl) of a given lock class (e.g. iolock) are held. > > > > Looking at the lockdep_is_held_type implementation, and our existing > > code for i_rwsem I really don't see the point of the extra shared > > check. Something like: > > > > static inline bool > > __xfs_rwsem_islocked( > > struct rw_semaphore *rwsem, > > bool excl) > > { > > if (rwsem_is_locked(rwsem)) { > > if (debug_locks && excl) > > return lockdep_is_held_type(rwsem, 1); > > return true; > > } > > > > return false; > > } > > > > should be all that we really need. > > I think that's a lot more clear. In addition to the slight confusion over a (true, true) > set of args, the current proposal also has the extra confusion of what happens if we pass > (false, false), for example. > > One other thought, since debug_locks getting turned off by lockdep means that > an exclusive test reverts to a shared|exclusive test, would it be worth adding > a WARN_ON_ONCE to make it clear when xfs rwsem lock testing coverage has been > reduced? > > -Eric > OK, thanks for the comments. Eric in the following code is WARN_ONCE() used as you suggested or did you have something else in mind? static inline bool __xfs_rwsem_islocked( struct rw_semaphore *rwsem, bool excl) { if (!rwsem_is_locked(rwsem)) { return false; } if (excl) { if (debug_locks) { return lockdep_is_held_type(rwsem, 1); } WARN_ONCE(1, "xfs rwsem lock testing coverage has been reduced\n"); } return true; }
On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote: > OK, thanks for the comments. > > Eric in the following code is WARN_ONCE() used as you suggested or did > you have something else in mind? > > static inline bool > __xfs_rwsem_islocked( > struct rw_semaphore *rwsem, > bool excl) > { > if (!rwsem_is_locked(rwsem)) { > return false; > } > > if (excl) { > if (debug_locks) { > return lockdep_is_held_type(rwsem, 1); > } > WARN_ONCE(1, > "xfs rwsem lock testing coverage has been reduced\n"); > } Yikes, hell no. This means every debug xfs build without lockdep will be full of warnings all the time.
On 2/20/20 10:32 AM, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote: >> OK, thanks for the comments. >> >> Eric in the following code is WARN_ONCE() used as you suggested or did >> you have something else in mind? >> >> static inline bool >> __xfs_rwsem_islocked( >> struct rw_semaphore *rwsem, >> bool excl) >> { >> if (!rwsem_is_locked(rwsem)) { >> return false; >> } >> >> if (excl) { >> if (debug_locks) { >> return lockdep_is_held_type(rwsem, 1); >> } >> WARN_ONCE(1, >> "xfs rwsem lock testing coverage has been reduced\n"); >> } > > Yikes, hell no. This means every debug xfs build without lockdep > will be full of warnings all the time. Let's just drop my suggestion for now, no need to further complicate the series at this time. -Eric
On Thu, Feb 20, 2020 at 08:32:32AM -0800, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote: > > OK, thanks for the comments. > > > > Eric in the following code is WARN_ONCE() used as you suggested or did > > you have something else in mind? > > > > static inline bool > > __xfs_rwsem_islocked( > > struct rw_semaphore *rwsem, > > bool excl) > > { > > if (!rwsem_is_locked(rwsem)) { > > return false; > > } > > > > if (excl) { > > if (debug_locks) { > > return lockdep_is_held_type(rwsem, 1); > > } > > WARN_ONCE(1, > > "xfs rwsem lock testing coverage has been reduced\n"); > > } > > Yikes, hell no. This means every debug xfs build without lockdep > will be full of warnings all the time. Well... once per module load, but if you /were/ going to go down this route I'd at least gate the warning on IS_ENABLED(CONFIG_LOCKDEP) so that we only get this one-time warning when lockdep is enabled but dies anyway, so that we'll know that we're running with half a brain. --D
On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote: > > > > +static inline bool > > > > +__xfs_rwsem_islocked( > > > > + struct rw_semaphore *rwsem, > > > > + bool shared, > > > > + bool excl) > > > > +{ > > > > + bool locked = false; > > > > + > > > > + if (!rwsem_is_locked(rwsem)) > > > > + return false; > > > > + > > > > + if (!debug_locks) > > > > + return true; > > > > + > > > > + if (shared) > > > > + locked = lockdep_is_held_type(rwsem, 0); > > > > + > > > > + if (excl) > > > > + locked |= lockdep_is_held_type(rwsem, 1); > > > > + > > > > + return locked; > > > > > > This could use some comments explaining the logic, especially why we > > > need the shared and excl flags, which seems very confusing given that > > > a lock can be held either shared or exclusive, but not neither and not > > > both. > > > > Yes, this predicate should document that callers are allowed to pass in > > shared==true and excl==true when the caller wants to assert that either > > lock type (shared or excl) of a given lock class (e.g. iolock) are held. > > Looking at the lockdep_is_held_type implementation, and our existing > code for i_rwsem I really don't see the point of the extra shared > check. Something like: > > static inline bool > __xfs_rwsem_islocked( > struct rw_semaphore *rwsem, > bool excl) > { > if (rwsem_is_locked(rwsem)) { > if (debug_locks && excl) > return lockdep_is_held_type(rwsem, 1); > return true; > } > > return false; > } > > should be all that we really need. > You don't see the point of extra shared check, but if we want to check that the semaphore is locked for reading and not writing? Having the semaphore locked for writing would make the code safe from race condition but could be a performance hit, right? Thanks for comments.
On 2/21/20 11:49 AM, Pavel Reichl wrote: > On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote: >>>>> +static inline bool >>>>> +__xfs_rwsem_islocked( >>>>> + struct rw_semaphore *rwsem, >>>>> + bool shared, >>>>> + bool excl) >>>>> +{ >>>>> + bool locked = false; >>>>> + >>>>> + if (!rwsem_is_locked(rwsem)) >>>>> + return false; >>>>> + >>>>> + if (!debug_locks) >>>>> + return true; >>>>> + >>>>> + if (shared) >>>>> + locked = lockdep_is_held_type(rwsem, 0); >>>>> + >>>>> + if (excl) >>>>> + locked |= lockdep_is_held_type(rwsem, 1); >>>>> + >>>>> + return locked; >>>> >>>> This could use some comments explaining the logic, especially why we >>>> need the shared and excl flags, which seems very confusing given that >>>> a lock can be held either shared or exclusive, but not neither and not >>>> both. >>> >>> Yes, this predicate should document that callers are allowed to pass in >>> shared==true and excl==true when the caller wants to assert that either >>> lock type (shared or excl) of a given lock class (e.g. iolock) are held. >> >> Looking at the lockdep_is_held_type implementation, and our existing >> code for i_rwsem I really don't see the point of the extra shared >> check. Something like: >> >> static inline bool >> __xfs_rwsem_islocked( >> struct rw_semaphore *rwsem, >> bool excl) >> { >> if (rwsem_is_locked(rwsem)) { >> if (debug_locks && excl) >> return lockdep_is_held_type(rwsem, 1); >> return true; >> } >> >> return false; >> } >> >> should be all that we really need. >> > > You don't see the point of extra shared check, but if we want to check > that the semaphore is locked for reading and not writing? Having the > semaphore locked for writing would make the code safe from race > condition but could be a performance hit, right? So, I raised this question with Pavel but I think maybe it was borne of my misunderstanding. Ok let me think this through. Today we have: int xfs_isilocked( xfs_inode_t *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 we assert xfs_isilocked(ip, XFS_ILOCK_SHARED) I guess we /already/ get a positive result if the inode is actually locked XFS_ILOCK_EXCL. So perhaps Christoph's suggestion really just keeps implementing what we already have today. It might be a reasonable question re: whether we ever want to know that we are locked shared and NOT locked exclusive, but we can't do that today, so I guess it shouldn't complicate this patchset. ... do I have this right? Thanks, -Eric
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index c5077e6326c7..3d28c4790231 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -345,32 +345,54 @@ xfs_ilock_demote( } #if defined(DEBUG) || defined(XFS_WARN) -int +static inline bool +__xfs_rwsem_islocked( + struct rw_semaphore *rwsem, + bool shared, + bool excl) +{ + bool locked = false; + + if (!rwsem_is_locked(rwsem)) + return false; + + if (!debug_locks) + return true; + + if (shared) + locked = lockdep_is_held_type(rwsem, 0); + + if (excl) + locked |= lockdep_is_held_type(rwsem, 1); + + return locked; +} + +bool xfs_isilocked( - xfs_inode_t *ip, + struct xfs_inode *ip, uint lock_flags) { - if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { - if (!(lock_flags & XFS_ILOCK_SHARED)) - return !!ip->i_lock.mr_writer; - return rwsem_is_locked(&ip->i_lock.mr_lock); + if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) { + return __xfs_rwsem_islocked(&ip->i_lock.mr_lock, + (lock_flags & XFS_ILOCK_SHARED), + (lock_flags & XFS_ILOCK_EXCL)); } - if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) { - if (!(lock_flags & XFS_MMAPLOCK_SHARED)) - return !!ip->i_mmaplock.mr_writer; - return rwsem_is_locked(&ip->i_mmaplock.mr_lock); + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) { + return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock, + (lock_flags & XFS_MMAPLOCK_SHARED), + (lock_flags & XFS_MMAPLOCK_EXCL)); } - if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) { - if (!(lock_flags & XFS_IOLOCK_SHARED)) - return !debug_locks || - lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0); - return rwsem_is_locked(&VFS_I(ip)->i_rwsem); + if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) { + return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, + (lock_flags & XFS_IOLOCK_SHARED), + (lock_flags & XFS_IOLOCK_EXCL)); } ASSERT(0); - return 0; + return false; } #endif diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 492e53992fa9..3d7ce355407d 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -416,7 +416,7 @@ void xfs_ilock(xfs_inode_t *, uint); int xfs_ilock_nowait(xfs_inode_t *, uint); void xfs_iunlock(xfs_inode_t *, uint); void xfs_ilock_demote(xfs_inode_t *, uint); -int xfs_isilocked(xfs_inode_t *, uint); +bool xfs_isilocked(xfs_inode_t *, uint); uint xfs_ilock_data_map_shared(struct xfs_inode *); uint xfs_ilock_attr_map_shared(struct xfs_inode *);
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> --- fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++-------------- fs/xfs/xfs_inode.h | 2 +- 2 files changed, 39 insertions(+), 17 deletions(-)