Re: [PATCH 07/13] xfs: Convert to use invalidate_lock
flat view
From: Dave Chinner <david@fromorbit.com>
Date: 2021-05-25 21:40:52
Also in:
ceph-devel, linux-cifs, linux-f2fs-devel, linux-fsdevel, linux-mm, linux-xfs
On Tue, May 25, 2021 at 03:50:44PM +0200, Jan Kara wrote:
quoted hunk ↗ jump to hunk
Use invalidate_lock instead of XFS internal i_mmap_lock. The intended purpose of invalidate_lock is exactly the same. Note that the locking in __xfs_filemap_fault() slightly changes as filemap_fault() already takes invalidate_lock. Reviewed-by: Christoph Hellwig <hch@lst.de> CC: <redacted> CC: "Darrick J. Wong" <redacted> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/xfs/xfs_file.c | 12 ++++++----- fs/xfs/xfs_inode.c | 52 ++++++++++++++++++++++++++-------------------- fs/xfs/xfs_inode.h | 1 - fs/xfs/xfs_super.c | 2 -- 4 files changed, 36 insertions(+), 31 deletions(-)diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 396ef36dcd0a..dc9cb5c20549 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c@@ -1282,7 +1282,7 @@ xfs_file_llseek( * * mmap_lock (MM) * sb_start_pagefault(vfs, freeze) - * i_mmaplock (XFS - truncate serialisation) + * invalidate_lock (vfs/XFS_MMAPLOCK - truncate serialisation) * page_lock (MM) * i_lock (XFS - extent map serialisation) */@@ -1303,24 +1303,26 @@ __xfs_filemap_fault( file_update_time(vmf->vma->vm_file); } - xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED); if (IS_DAX(inode)) { pfn_t pfn; + xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED); ret = dax_iomap_fault(vmf, pe_size, &pfn, NULL, (write_fault && !vmf->cow_page) ? &xfs_direct_write_iomap_ops : &xfs_read_iomap_ops); if (ret & VM_FAULT_NEEDDSYNC) ret = dax_finish_sync_fault(vmf, pe_size, pfn); + xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED); } else { - if (write_fault) + if (write_fault) { + xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED); ret = iomap_page_mkwrite(vmf, &xfs_buffered_write_iomap_ops); - else + xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED); + } else ret = filemap_fault(vmf); } - xfs_iunlock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
This seems kinda messy. filemap_fault() basically takes the
invalidate lock around the entire operation, it runs, so maybe it
would be cleaner to implement it as:
filemap_fault_locked(vmf)
{
/* does the filemap fault work */
}
filemap_fault(vmf)
{
filemap_invalidate_down_read(...)
ret = filemap_fault_locked(vmf)
filemap_invalidate_up_read(...)
return ret;
}
And that means XFS could just call filemap_fault_locked() and not
have to do all this messy locking just to avoid holding the lock
that filemap_fault has now internalised.
quoted hunk ↗ jump to hunk
@@ -355,8 +358,11 @@ xfs_isilocked( 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); + return !debug_locks || + lockdep_is_held_type( + &VFS_I(ip)->i_mapping->invalidate_lock, + 0); + return rwsem_is_locked(&VFS_I(ip)->i_mapping->invalidate_lock); }
<sigh> And so here we are again, losing more of our read vs write debug checks on debug kernels when lockdep is not enabled.... Can we please add rwsem_is_locked_read() and rwsem_is_locked_write() wrappers that just look at the rwsem counter value to determine how the lock is held? Then the mrlock_t can go away entirely.... Cheers, Dave. -- Dave Chinner david@fromorbit.com