From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay3.corp.sgi.com [198.149.34.15]) by oss.sgi.com (Postfix) with ESMTP id CDDC57CA2 for ; Thu, 11 Feb 2016 10:22:18 -0600 (CST) Received: from cuda.sgi.com (cuda2.sgi.com [192.48.176.25]) by relay3.corp.sgi.com (Postfix) with ESMTP id 51B40AC003 for ; Thu, 11 Feb 2016 08:22:15 -0800 (PST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by cuda.sgi.com with ESMTP id OPBlvCK6DVGHariU (version=TLSv1 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 11 Feb 2016 08:22:12 -0800 (PST) Date: Thu, 11 Feb 2016 17:22:26 +0100 From: Jan Kara Subject: Re: [PATCH v2 2/2] dax: move writeback calls into the filesystems Message-ID: <20160211162226.GR21760@quack.suse.cz> References: <1455137336-28720-1-git-send-email-ross.zwisler@linux.intel.com> <1455137336-28720-3-git-send-email-ross.zwisler@linux.intel.com> <20160210220312.GP14668@dastard> <20160210224340.GA30938@linux.intel.com> <20160211125044.GJ21760@quack.suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Dan Williams Cc: Theodore Ts'o , "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" , Matthew Wilcox , XFS Developers , Linux MM , Andreas Dilger , Alexander Viro , Jan Kara , linux-fsdevel , Jan Kara , Ross Zwisler , linux-ext4 , Andrew Morton On Thu 11-02-16 07:22:00, Dan Williams wrote: > On Thu, Feb 11, 2016 at 4:50 AM, Jan Kara wrote: > > On Wed 10-02-16 15:43:40, Ross Zwisler wrote: > >> On Thu, Feb 11, 2016 at 09:03:12AM +1100, Dave Chinner wrote: > >> > On Wed, Feb 10, 2016 at 01:48:56PM -0700, Ross Zwisler wrote: > >> > > Previously calls to dax_writeback_mapping_range() for all DAX filesystems > >> > > (ext2, ext4 & xfs) were centralized in filemap_write_and_wait_range(). > >> > > dax_writeback_mapping_range() needs a struct block_device, and it used to > >> > > get that from inode->i_sb->s_bdev. This is correct for normal inodes > >> > > mounted on ext2, ext4 and XFS filesystems, but is incorrect for DAX raw > >> > > block devices and for XFS real-time files. > >> > > > >> > > Instead, call dax_writeback_mapping_range() directly from the filesystem > >> > > ->writepages function so that it can supply us with a valid block > >> > > device. This also fixes DAX code to properly flush caches in response to > >> > > sync(2). > >> > > > >> > > Signed-off-by: Ross Zwisler > >> > > Signed-off-by: Jan Kara > >> > > --- > >> > > fs/block_dev.c | 16 +++++++++++++++- > >> > > fs/dax.c | 13 ++++++++----- > >> > > fs/ext2/inode.c | 11 +++++++++++ > >> > > fs/ext4/inode.c | 7 +++++++ > >> > > fs/xfs/xfs_aops.c | 9 +++++++++ > >> > > include/linux/dax.h | 6 ++++-- > >> > > mm/filemap.c | 12 ++++-------- > >> > > 7 files changed, 58 insertions(+), 16 deletions(-) > >> > > > >> > > diff --git a/fs/block_dev.c b/fs/block_dev.c > >> > > index 39b3a17..fc01e43 100644 > >> > > --- a/fs/block_dev.c > >> > > +++ b/fs/block_dev.c > >> > > @@ -1693,13 +1693,27 @@ static int blkdev_releasepage(struct page *page, gfp_t wait) > >> > > return try_to_free_buffers(page); > >> > > } > >> > > > >> > > +static int blkdev_writepages(struct address_space *mapping, > >> > > + struct writeback_control *wbc) > >> > > +{ > >> > > + if (dax_mapping(mapping)) { > >> > > + struct block_device *bdev = I_BDEV(mapping->host); > >> > > + int error; > >> > > + > >> > > + error = dax_writeback_mapping_range(mapping, bdev, wbc); > >> > > + if (error) > >> > > + return error; > >> > > + } > >> > > + return generic_writepages(mapping, wbc); > >> > > +} > >> > > >> > Can you remind of the reason for calling generic_writepages() on DAX > >> > enabled address spaces? > >> > >> Sure. The initial version of this patch didn't do this, and during testing I > >> hit a bunch of xfstests failures. In ext2 at least I believe these were > >> happening because we were skipping the call into generic_writepages() for DAX > >> inodes. Without a lot of data to back this up, my guess is that this is due > >> to metadata inodes or something being marked as DAX (so dax_mapping(mapping) > >> returns true), but having dirty page cache pages that need to be written back > >> as part of the writeback. > >> > >> Changing this so we always call generic_writepages() even in the DAX case > >> solved the xfstest failures. > >> > >> If this sounds incorrect, please let me know and I'll go and gather more data. > > > > So I think a more correct fix it to not set S_DAX for inodes that will have > > any pagecache pages - e.g. don't set S_DAX for block device inodes when > > filesystem is mounted on it (probably the easiest is to just refuse to > > mount filesystem on block device which has S_DAX set). > > I think we have a wider problem here. See __blkdev_get, we set S_DAX > on all block devices that have ->direct_access() and have a > page-aligned starting address. It seems to me we need to modify the > metadata i/o paths to bypass the page cache Heh, no way to do that easily. All the journalling machinery depends on buffers and pages... >, or teach the fsync code > how to flush populated data pages out of the radix. This might be doable but it will be difficult to avoid aliasing issues and data corruption. And mainly I don't see the point: When you mount a filesystem on top of block device, you do not want to mess with the block device directly, even less using DAX. So we just have to find a way how to set S_DAX for normal open but clear it from fs path. At worst, we could clear S_DAX on the block device in mount_bdev() or something like that... Honza -- Jan Kara SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs