On Mon 07-06-21 08:56:33, Darrick J. Wong wrote:
> On Mon, Jun 07, 2021 at 04:52:18PM +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@xxxxxx>
> > CC: <linux-xfs@xxxxxxxxxxxxxxx>
> > CC: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > fs/xfs/xfs_file.c | 13 +++++++-----
> > fs/xfs/xfs_inode.c | 50 ++++++++++++++++++++++++----------------------
> > fs/xfs/xfs_inode.h | 1 -
> > fs/xfs/xfs_super.c | 2 --
> > 4 files changed, 34 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 396ef36dcd0a..7cb7703c2209 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,27 @@ __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);
>
> I've been wondering if iomap_page_mkwrite and dax_iomap_fault should be
> taking these locks? I guess that would violate the premise that iomap
> requires that callers arrange for concurrency control (i.e. iomap
> doesn't take locks).
Well, iomap does take page locks but I agree that generally it stays away
from high-level locks. So keeping invalidate_lock out of it makes more
sense to me as well.
> Code changes look fine, though.
>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
Thanks!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
