From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965208AbcBBSxx (ORCPT ); Tue, 2 Feb 2016 13:53:53 -0500 Received: from mga03.intel.com ([134.134.136.65]:60032 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965044AbcBBSxu (ORCPT ); Tue, 2 Feb 2016 13:53:50 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,385,1449561600"; d="scan'208";a="894918293" Date: Tue, 2 Feb 2016 11:53:41 -0700 From: Ross Zwisler To: Ross Zwisler , Jan Kara , Dave Chinner , Matthew Wilcox , Christoph Hellwig , linux-kernel@vger.kernel.org, Alexander Viro , Andrew Morton , Dan Williams , Jan Kara , linux-fsdevel@vger.kernel.org, linux-nvdimm@ml01.01.org Subject: Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences Message-ID: <20160202185341.GA10496@linux.intel.com> Mail-Followup-To: Ross Zwisler , Jan Kara , Dave Chinner , Matthew Wilcox , Christoph Hellwig , linux-kernel@vger.kernel.org, Alexander Viro , Andrew Morton , Dan Williams , Jan Kara , linux-fsdevel@vger.kernel.org, linux-nvdimm@ml01.01.org References: <1454009704-25959-1-git-send-email-ross.zwisler@linux.intel.com> <1454009704-25959-2-git-send-email-ross.zwisler@linux.intel.com> <20160128213858.GA29114@infradead.org> <20160129182815.GB5224@linux.intel.com> <20160130052833.GY2948@linux.intel.com> <20160201145147.GD13740@quack.suse.cz> <20160201214730.GR20456@dastard> <20160202111723.GD12574@quack.suse.cz> <20160202184134.GA18486@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160202184134.GA18486@linux.intel.com> 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 Tue, Feb 02, 2016 at 11:41:34AM -0700, Ross Zwisler wrote: > On Tue, Feb 02, 2016 at 12:17:23PM +0100, Jan Kara wrote: > > On Tue 02-02-16 08:47:30, Dave Chinner wrote: > > > On Mon, Feb 01, 2016 at 03:51:47PM +0100, Jan Kara wrote: > > > > On Sat 30-01-16 00:28:33, Matthew Wilcox wrote: > > > > > On Fri, Jan 29, 2016 at 11:28:15AM -0700, Ross Zwisler wrote: > > > > > > I guess I need to go off and understand if we can have DAX mappings on such a > > > > > > device. If we can, we may have a problem - we can get the block_device from > > > > > > get_block() in I/O path and the various fault paths, but we don't have access > > > > > > to get_block() when flushing via dax_writeback_mapping_range(). We avoid > > > > > > needing it the normal case by storing the sector results from get_block() in > > > > > > the radix tree. > > > > > > > > > > I think we're doing it wrong by storing the sector in the radix tree; we'd > > > > > really need to store both the sector and the bdev which is too much data. > > > > > > > > > > If we store the PFN of the underlying page instead, we don't have this > > > > > problem. Instead, we have a different problem; of the device going > > > > > away under us. I'm trying to find the code which tears down PTEs when > > > > > the device goes away, and I'm not seeing it. What do we do about user > > > > > mappings of the device? > > > > > > > > So I don't have a strong opinion whether storing PFN or sector is better. > > > > Maybe PFN is somewhat more generic but OTOH turning DAX off for special > > > > cases like inodes on XFS RT devices would be IMHO fine. > > > > > > We need to support alternate devices. > > > > > > There is a strong case for using the XFS RT device with DAX, > > > especially for applications that know they are going to always use > > > large/huge/giant pages to access their data files. The XFS RT device > > > can guarantee allocation is always aligned to large/huge/giant page > > > constraints right up to ENOSPC and throughout the production life of > > > the filesystem. We have no other filesystem capable of providing > > > such guarantees, which means the XFS RT device is uniquely suited to > > > certain aplications with DAX... > > > > I see, thanks for explanation. So I'm OK with changing what is stored in > > the radix tree to accommodate this use case but my reservation that we IHMO > > have other more pressing things to fix remains... > > IMO this is pretty pressing - without it neither XFS RT devices nor DAX raw > block devices work. The case has been made above for XFS RT devices, and with > DAX raw block devices we really need a fix because the current code will cause > a kernel BUG when a user tries to fsync/msync a raw block device mmap(). This > is especially bad because, unlike with filesystems where you mount with the > dax mount option, there is no opt-in step for raw block devices. > > This has to be fixed - it seems like we either figure out how to fix DAX > fsync, or we have to disable DAX on raw block devices for a kernel cycle. I'm > hoping for the former. :) Well, I guess a third option would be to keep DAX raw block device in and just take this patch as a temporary fix: https://lkml.org/lkml/2016/1/28/679 This would leave XFS RT broken, though, so we may want to explicitly disable DAX + XFS RT configs for now, but at least we wouldn't have the raw block device kernel BUG.