| Message ID | 20210607145236.31852-7-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Hole punch vs page cache filling races | expand |
On Mon, Jun 07, 2021 at 04:52:17PM +0200, Jan Kara wrote: > From: Pavel Reichl <preichl@redhat.com> > > 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> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/xfs/xfs_inode.c | 39 +++++++++++++++++++++++++++++++-------- > fs/xfs/xfs_inode.h | 21 ++++++++++++++------- > 2 files changed, 45 insertions(+), 15 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 0369eb22c1bb..6247977870bd 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -342,9 +342,34 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + int lock_flags, > + int shift) > +{ > + lock_flags >>= shift; > + > + if (!debug_locks) > + return rwsem_is_locked(rwsem); > + /* > + * 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)." Extra double-quote not needed here. With that fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> (You can delete the previous review tag.) --D > + */ > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + return lockdep_is_held_type(rwsem, -1); > + } > + return lockdep_is_held_type(rwsem, 0); > +} > + > +bool > xfs_isilocked( > - xfs_inode_t *ip, > + struct xfs_inode *ip, > uint lock_flags) > { > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { > @@ -359,15 +384,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 ca826cfba91c..1c0e15c480bc 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -262,12 +262,19 @@ static inline bool xfs_inode_has_bigtime(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 \ > @@ -410,7 +417,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, Jun 07, 2021 at 04:52:17PM +0200, Jan Kara wrote: > From: Pavel Reichl <preichl@redhat.com> > > 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> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/xfs/xfs_inode.c | 39 +++++++++++++++++++++++++++++++-------- > fs/xfs/xfs_inode.h | 21 ++++++++++++++------- > 2 files changed, 45 insertions(+), 15 deletions(-) As a standalone patch, this is overly elaborate and way more complex than it needs to be. It's not really just a refactor, either, because of the unnecessary shifting games it adds. > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 0369eb22c1bb..6247977870bd 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -342,9 +342,34 @@ xfs_ilock_demote( > } > > #if defined(DEBUG) || defined(XFS_WARN) > -int > +static inline bool > +__xfs_rwsem_islocked( > + struct rw_semaphore *rwsem, > + int lock_flags, > + int shift) > +{ > + lock_flags >>= shift; > + > + if (!debug_locks) > + return rwsem_is_locked(rwsem); > + /* > + * 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)." > + */ > + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { > + return lockdep_is_held_type(rwsem, -1); > + } > + return lockdep_is_held_type(rwsem, 0); > +} Pass in a boolean value for shared/exclusive and you can get rid of passing in the lock flags as well. static bool __xfs_rwsem_islocked( struct rw_semaphore *rwsem, bool shared) { if (!debug_locks) return rwsem_is_locked(rwsem); if (!shared) return lockdep_is_held_type(rwsem, 0); /* * We are checking that the lock is held at least in shared * mode but don't care that it might be held exclusively * (i.e. shared | excl). Hence we check if the lock is held * in any mode rather than an explicit shared mode. */ return lockdep_is_held_type(rwsem, -1); } > + > +bool > xfs_isilocked( > - xfs_inode_t *ip, > + struct xfs_inode *ip, > uint lock_flags) > { > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { > @@ -359,15 +384,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); Then this is simply: return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, (lock_flags & XFS_IOLOCK_SHARED)); And the conversion for the MMAPLOCK in the next patch is equally simple. > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index ca826cfba91c..1c0e15c480bc 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -262,12 +262,19 @@ static inline bool xfs_inode_has_bigtime(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 \ And all this shifting goes away and the change is much, much simpler. If/when other changes are made to this code that require shifting like this, then we can make these modifications. But in this patch for this usage they don't really make much sense at all.. Cheers, Dave.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 0369eb22c1bb..6247977870bd 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -342,9 +342,34 @@ xfs_ilock_demote( } #if defined(DEBUG) || defined(XFS_WARN) -int +static inline bool +__xfs_rwsem_islocked( + struct rw_semaphore *rwsem, + int lock_flags, + int shift) +{ + lock_flags >>= shift; + + if (!debug_locks) + return rwsem_is_locked(rwsem); + /* + * 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)." + */ + if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) { + return lockdep_is_held_type(rwsem, -1); + } + return lockdep_is_held_type(rwsem, 0); +} + +bool xfs_isilocked( - xfs_inode_t *ip, + struct xfs_inode *ip, uint lock_flags) { if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) { @@ -359,15 +384,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 ca826cfba91c..1c0e15c480bc 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -262,12 +262,19 @@ static inline bool xfs_inode_has_bigtime(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 \ @@ -410,7 +417,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 *);