From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B48ABC47087 for ; Tue, 25 May 2021 21:40:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 927AF613D5 for ; Tue, 25 May 2021 21:40:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232146AbhEYVmV (ORCPT ); Tue, 25 May 2021 17:42:21 -0400 Received: from mail109.syd.optusnet.com.au ([211.29.132.80]:56346 "EHLO mail109.syd.optusnet.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231801AbhEYVmU (ORCPT ); Tue, 25 May 2021 17:42:20 -0400 Received: from dread.disaster.area (pa49-180-230-185.pa.nsw.optusnet.com.au [49.180.230.185]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id F04E9671F3; Wed, 26 May 2021 07:40:43 +1000 (AEST) Received: from dave by dread.disaster.area with local (Exim 4.92.3) (envelope-from ) id 1llen3-005CSi-MJ; Wed, 26 May 2021 07:40:41 +1000 Date: Wed, 26 May 2021 07:40:41 +1000 From: Dave Chinner To: Jan Kara Cc: linux-fsdevel@vger.kernel.org, Christoph Hellwig , ceph-devel@vger.kernel.org, Chao Yu , Damien Le Moal , "Darrick J. Wong" , Jaegeuk Kim , Jeff Layton , Johannes Thumshirn , linux-cifs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-mm@kvack.org, linux-xfs@vger.kernel.org, Miklos Szeredi , Steve French , Ted Tso , Matthew Wilcox , Christoph Hellwig Subject: Re: [PATCH 07/13] xfs: Convert to use invalidate_lock Message-ID: <20210525214041.GJ664593@dread.disaster.area> References: <20210525125652.20457-1-jack@suse.cz> <20210525135100.11221-7-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210525135100.11221-7-jack@suse.cz> X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.3 cv=Tu+Yewfh c=1 sm=1 tr=0 a=dUIOjvib2kB+GiIc1vUx8g==:117 a=dUIOjvib2kB+GiIc1vUx8g==:17 a=kj9zAlcOel0A:10 a=5FLXtPjwQuUA:10 a=VwQbUJbxAAAA:8 a=yPCof4ZbAAAA:8 a=7-415B0cAAAA:8 a=YRNIVghP3Sa-aXUPf-oA:9 a=CjuIK1q_8ugA:10 a=AjGcO6oz07-iQ99wixmX:22 a=biEYGPWJfzWAr4FL6Ov7:22 Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org 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 > CC: > CC: "Darrick J. Wong" > Signed-off-by: Jan Kara > --- > 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); > } 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