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 15:42:32 +0200 From: Jan Kara <jack@...e.cz> To: Dave Chinner <david@...morbit.com> Cc: Jan Kara <jack@...e.cz>, 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 Wed 26-05-21 12:20:59, Jan Kara wrote: > On Wed 26-05-21 07:40:41, Dave Chinner wrote: > > 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. > > Sure, I can do that. Hum, looking into this in more detail it isn't as easy. There are some operations inside filemap_fault() that need to be done outside of invalidate_lock. In particular we call into readahead code which will grab invalidate_lock for itself. So we'd need to pass in struct readahead_control whether invalidate_lock is held or not which is IMHO uglier than what we currently do in __xfs_filemap_fault(). Honza -- Jan Kara <jack@...e.com> SUSE Labs, CR
Powered by blists - more mailing lists