From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753250AbcBGF1d (ORCPT ); Sun, 7 Feb 2016 00:27:33 -0500 Received: from mga04.intel.com ([192.55.52.120]:37553 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751434AbcBGF1b (ORCPT ); Sun, 7 Feb 2016 00:27:31 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,408,1449561600"; d="scan'208";a="43762571" Date: Sat, 6 Feb 2016 22:27:23 -0700 From: Ross Zwisler To: Dave Chinner Cc: Jan Kara , Ross Zwisler , Dan Williams , Matthew Wilcox , Christoph Hellwig , "linux-kernel@vger.kernel.org" , Alexander Viro , Andrew Morton , linux-fsdevel , linux-nvdimm Subject: Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Message-ID: <20160207052723.GA26420@linux.intel.com> Mail-Followup-To: Ross Zwisler , Dave Chinner , Jan Kara , Dan Williams , Matthew Wilcox , Christoph Hellwig , "linux-kernel@vger.kernel.org" , Alexander Viro , Andrew Morton , linux-fsdevel , linux-nvdimm References: <20160201214730.GR20456@dastard> <20160202111723.GD12574@quack.suse.cz> <20160202164642.GE12574@quack.suse.cz> <20160202173456.GB23963@linux.intel.com> <20160203104611.GF12574@quack.suse.cz> <20160203201328.GC29275@linux.intel.com> <20160204091558.GB4956@quack.suse.cz> <20160206231512.GF31407@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160206231512.GF31407@dastard> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 07, 2016 at 10:15:12AM +1100, Dave Chinner wrote: > On Thu, Feb 04, 2016 at 10:15:58AM +0100, Jan Kara wrote: > > On Wed 03-02-16 13:13:28, Ross Zwisler wrote: > > > Here is the comment from Dave Chinner that had me move to having the calls to > > > dax_writeback_mapping_range() into the generic mm code: > > > > > > > Lastly, this flushing really needs to be inside > > > > filemap_write_and_wait_range(), because we call the writeback code > > > > from many more places than just fsync to ensure ordering of various > > > > operations such that files are in known state before proceeding > > > > (e.g. hole punch). > > > https://lkml.org/lkml/2015/11/16/847 > ..... > > > > So revisiting the decision I see two options: > > > > > > > > 1) Move the DAX flushing code from filemap_write_and_wait() into > > > > ->writepages() fs callback. There the filesystem can provide all the > > > > information it needs including bdev, get_block callback, or whatever. > > > > > > This seems fine as long as we add it to ->fsync as well since ->writepages is > > > never called in that path, and as long as we are okay with skipping DAX > > > writebacks on hole punch, truncate, and block relocation. > > > > Look at ext4_sync_file() -> filemap_write_and_wait_range() -> > > __filemap_fdatawrite_range() -> do_writepages(). Except those nrpages > 0 > > checks which would need to be changed. > > Just to be clear: this is pretty much what I was implying was > necessary when I said that the DAX flushing needed to be "inside > filemap_write_and_wait_range". And that's what I thought Ross was > planning on doing after that round discussion. i.e. Ross said: > > "If the race described above isn't an issue then I agree moving this > call out of the filesystems and down into the generic page writeback > code is probably the right thing to do." > > https://lkml.org/lkml/2015/11/17/718 > > Realistically, what Jan is saying in this thread is exactly what I > said we needed to do way back when I first pointed out that fsync > was broken and dirty tracking in the mapping radix tree was still > needed for fsync to work effectively. Here, let me try and quickly summarize what is going on. 1) The DAX fsync set was merged into v4.5-rc1, it does use the radix tree for tracking dirty PTE and PMD pages, and we do currently call into the DAX sync code via filemap_write_and_wait_range() as you initially suggested. 2) During testing of raw block devices + DAX I noticed that the struct block_device that we were using for DAX operations was incorrect. For the fault handlers, etc. we can just get the correct bdev via get_block(), which is passed in as a function pointer, but for the flushing code we don't have access to get_block(). This is also an issue for XFS real-time devices, whenever we get those working. In short, somehow we need to get dax_writeback_mapping_range() a valid bdev. Right now it is called via filemap_write_and_wait_range(), which can't provide either the bdev nor a get_block() function pointer. So, our options seem to be: a) Move the calls to dax_writeback_mapping_range() into the filesystems (what Jan is suggesting, i.e. ->writepages()) b) Keep the calls to dax_writeback_mapping_range() in the mm code, and provide a generic way to ask a filesystem for an inode's bdev. I did a version of this using a superblock operation here: https://lkml.org/lkml/2016/2/2/941 3) During the review and discussion for the above problems, Jan noticed that the flushing code wasn't being called for sync() and syncfs(). Clearly from your other response (https://lkml.org/lkml/2016/2/6/168) you think this is incorrect. Regardless, the above issue remains - dax_writeback_mapping_range() needs a bdev. Do we move the calls into the filesystem so the fs can provide a bdev, or do we we create a generic method for DAX to ask the fs for the correct bdev for an inode?