From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Zwisler Subject: Re: [RFC PATCH 2/2] block: enable dax for raw block devices Date: Sun, 18 Oct 2015 21:01:30 -0600 Message-ID: <20151019030130.GA23284@linux.intel.com> References: <20151017004653.2742.36299.stgit@dwillia2-desk3.amr.corp.intel.com> <20151017004702.2742.82530.stgit@dwillia2-desk3.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-nvdimm@lists.01.org, Xiao Guangrong , kvm@vger.kernel.org, Jeff Moyer , Al Viro , Ross Zwisler , Andrew Morton , Christoph Hellwig To: Dan Williams Return-path: Received: from mga09.intel.com ([134.134.136.24]:50265 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752440AbbJSDBb (ORCPT ); Sun, 18 Oct 2015 23:01:31 -0400 Content-Disposition: inline In-Reply-To: <20151017004702.2742.82530.stgit@dwillia2-desk3.amr.corp.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Oct 16, 2015 at 08:49:41PM -0400, Dan Williams wrote: > If an application wants exclusive access to all of the persistent memory > provided by an NVDIMM namespace it can use this raw-block-dax facility > to forgo establishing a filesystem. This capability is targeted > primarily to hypervisors wanting to provision persistent memory for > guests. > > Cc: Jeff Moyer > Cc: Christoph Hellwig > Cc: Al Viro > Cc: Andrew Morton > Cc: Ross Zwisler > Cc: Xiao Guangrong > Signed-off-by: Dan Williams > --- > > Only lighted tested so far, but seems to work, is the shortest path to a > DAX mapping, and makes it easier to trigger the pmd_fault path (no > fs-block-allocator interactions). > > fs/block_dev.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 1 deletion(-) > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 5277dd83d254..498b71455570 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -1687,13 +1687,95 @@ static const struct address_space_operations def_blk_aops = { > .is_dirty_writeback = buffer_check_dirty_writeback, > }; > > +#ifdef CONFIG_FS_DAX > +static int blkdev_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > +{ > + struct inode *bd_inode = file_bd_inode(vma->vm_file); > + struct block_device *bdev = I_BDEV(bd_inode); > + int ret; > + > + mutex_lock(&bdev->bd_mutex); > + ret = __dax_fault(vma, vmf, blkdev_get_block, NULL); > + mutex_unlock(&bdev->bd_mutex); > + > + return ret; > +} This all looks very straightforward. The one comment I have is that this code is missing the calls to sb_[start|end]_pagefault(), and to file_update_time() that are found in ext[24]/xfs and the generic fault code. The previous version of this code used the generic fault implementation, and was calling these functions via filemap_page_mkwrite(). It is possible that they were omitted for a reason - does protection from filesystem freezing still make sense when talking with a raw block device? For example, if that block device *has* a mounted filesystem on it that is frozen, does sb_start_pagefault() prevent against page faults on the raw device that try and make something writable? In any case, the presence of them in filemap_page_mkwrite() tells me that they at least aren't harmful, and I wanted to make sure they weren't needed before leaving them out. If the omission was intentional, should we add a comment to explain why they are missing?