lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 26 May 2021 07:40:41 +1000 From: Dave Chinner <david@...morbit.com> To: Jan Kara <jack@...e.cz> Cc: linux-fsdevel@...r.kernel.org, Christoph Hellwig <hch@...radead.org>, ceph-devel@...r.kernel.org, Chao Yu <yuchao0@...wei.com>, Damien Le Moal <damien.lemoal@....com>, "Darrick J. Wong" <darrick.wong@...cle.com>, Jaegeuk Kim <jaegeuk@...nel.org>, Jeff Layton <jlayton@...nel.org>, Johannes Thumshirn <jth@...nel.org>, linux-cifs@...r.kernel.org, linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net, linux-mm@...ck.org, linux-xfs@...r.kernel.org, Miklos Szeredi <miklos@...redi.hu>, Steve French <sfrench@...ba.org>, Ted Tso <tytso@....edu>, Matthew Wilcox <willy@...radead.org>, Christoph Hellwig <hch@....de> Subject: Re: [PATCH 07/13] xfs: Convert to use invalidate_lock On Tue, May 25, 2021 at 03:50:44PM +0200, Jan Kara wrote: > 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@....de> > CC: <linux-xfs@...r.kernel.org> > CC: "Darrick J. Wong" <darrick.wong@...cle.com> > Signed-off-by: Jan Kara <jack@...e.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. > @@ -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@...morbit.com
Powered by blists - more mailing lists